Whamcloud - gitweb
LU-3093 lnet: Fix assert on empty group in selftest module 92/6092/16
authorAmir Shehata <amir.shehata@intel.com>
Wed, 10 Jul 2013 17:45:35 +0000 (10:45 -0700)
committerOleg Drokin <oleg.drokin@intel.com>
Mon, 22 Jul 2013 22:31:10 +0000 (22:31 +0000)
The core of the issue is that the selftest module doesn't sanitize its
own API, but it depends on lst utility to do such checks.  As a result
this issue manifests itself in this particular LU through an assert
on an empty group.  If the NID is misspelled then an empty group is
added.  An error output is provided, but if that's never checked in a
batch script, as is the case with this issue, then the script will try
to add an empty group to a test to run in a batch, and that will cause
an assert

The fix is two fold.  Ensure that lst utility checks that a group is
added with at least one node.  If not the group is subsequently
deleted.  And the add_test command would fail, since the group no
longer exists.

The second fix is to ensure that the kernel module itself sanitizes
its own API in this particular case, so that if a different utility is
used other than lst to communicate with the selftest kernel module
then this error would be caught.  This fix looks up the batch and the
groups, src and dst, in the ioctl handle and sanitizes that input at
this point.  If the group looked up either doesn't exist or doesn't
have at least one ACTIVE node, then the command fails.

NOTE:there are many other cases in the code where the selftest kernel
module doesn't check for sanity of the input, but depends totally on
the lst module to do such checks.  Particularly around length of
strings passed in.  Thus it is possible to crash the selftest module
if someone tries to create another userspace app to communicate with
the selftest kernel module without ensuring sanity of the params sent
to the kernel module.  In effect, it's always assumed that lst is the
front end for selftest and no other front end is to be used.

Signed-off-by: Amir Shehata <amir.shehata@intel.com>
Change-Id: If2e59c1fadf8bd6cac759ea8fa761be386e71bd3
Reviewed-on: http://review.whamcloud.com/6092
Tested-by: Hudson
Reviewed-by: Isaac Huang <he.huang@intel.com>
Reviewed-by: Liang Zhen <liang.zhen@intel.com>
Tested-by: Maloo <whamcloud.maloo@gmail.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
lnet/selftest/conctl.c
lnet/selftest/conrpc.c
lnet/selftest/console.c
lnet/selftest/console.h
lnet/utils/lst.c

index 68297bd..5d3a3fd 100644 (file)
@@ -715,25 +715,25 @@ lst_stat_query_ioctl(lstio_stat_args_t *args)
 
 int lst_test_add_ioctl(lstio_test_args_t *args)
 {
-        char           *name;
-        char           *srcgrp = NULL;
-        char           *dstgrp = NULL;
-        void           *param = NULL;
-        int             ret = 0;
-        int             rc = -ENOMEM;
-
-        if (args->lstio_tes_resultp == NULL ||
-            args->lstio_tes_retp == NULL ||
-            args->lstio_tes_bat_name == NULL || /* no specified batch */
-            args->lstio_tes_bat_nmlen <= 0 ||
-            args->lstio_tes_bat_nmlen > LST_NAME_SIZE ||
-            args->lstio_tes_sgrp_name == NULL || /* no source group */
-            args->lstio_tes_sgrp_nmlen <= 0 ||
-            args->lstio_tes_sgrp_nmlen > LST_NAME_SIZE ||
-            args->lstio_tes_dgrp_name == NULL || /* no target group */
-            args->lstio_tes_dgrp_nmlen <= 0 ||
-            args->lstio_tes_dgrp_nmlen > LST_NAME_SIZE)
-                return -EINVAL;
+       char            *batch_name;
+       char            *src_name = NULL;
+       char            *dst_name = NULL;
+       void            *param = NULL;
+       int             ret = 0;
+       int             rc = -ENOMEM;
+
+       if (args->lstio_tes_resultp == NULL ||
+           args->lstio_tes_retp == NULL ||
+           args->lstio_tes_bat_name == NULL || /* no specified batch */
+           args->lstio_tes_bat_nmlen <= 0 ||
+           args->lstio_tes_bat_nmlen > LST_NAME_SIZE ||
+           args->lstio_tes_sgrp_name == NULL || /* no source group */
+           args->lstio_tes_sgrp_nmlen <= 0 ||
+           args->lstio_tes_sgrp_nmlen > LST_NAME_SIZE ||
+           args->lstio_tes_dgrp_name == NULL || /* no target group */
+           args->lstio_tes_dgrp_nmlen <= 0 ||
+           args->lstio_tes_dgrp_nmlen > LST_NAME_SIZE)
+               return -EINVAL;
 
        if (args->lstio_tes_loop == 0 || /* negative is infinite */
            args->lstio_tes_concur <= 0 ||
@@ -748,60 +748,61 @@ int lst_test_add_ioctl(lstio_test_args_t *args)
             PAGE_CACHE_SIZE - sizeof(lstcon_test_t)))
                 return -EINVAL;
 
-        LIBCFS_ALLOC(name, args->lstio_tes_bat_nmlen + 1);
-        if (name == NULL)
-                return rc;
+       LIBCFS_ALLOC(batch_name, args->lstio_tes_bat_nmlen + 1);
+       if (batch_name == NULL)
+               return rc;
 
-        LIBCFS_ALLOC(srcgrp, args->lstio_tes_sgrp_nmlen + 1);
-        if (srcgrp == NULL) 
-                goto out;
+       LIBCFS_ALLOC(src_name, args->lstio_tes_sgrp_nmlen + 1);
+       if (src_name == NULL)
+               goto out;
 
-        LIBCFS_ALLOC(dstgrp, args->lstio_tes_dgrp_nmlen + 1);
-        if (dstgrp == NULL)
-                goto out;
+       LIBCFS_ALLOC(dst_name, args->lstio_tes_dgrp_nmlen + 1);
+        if (dst_name == NULL)
+               goto out;
 
-        if (args->lstio_tes_param != NULL) {
-                LIBCFS_ALLOC(param, args->lstio_tes_param_len);
-                if (param == NULL) 
-                        goto out;
-        }
+       if (args->lstio_tes_param != NULL) {
+               LIBCFS_ALLOC(param, args->lstio_tes_param_len);
+               if (param == NULL)
+                       goto out;
+       }
 
        rc = -EFAULT;
-       if (copy_from_user(name, args->lstio_tes_bat_name,
+       if (copy_from_user(batch_name, args->lstio_tes_bat_name,
                           args->lstio_tes_bat_nmlen) ||
-           copy_from_user(srcgrp, args->lstio_tes_sgrp_name,
+           copy_from_user(src_name, args->lstio_tes_sgrp_name,
                           args->lstio_tes_sgrp_nmlen) ||
-           copy_from_user(dstgrp, args->lstio_tes_dgrp_name,
+           copy_from_user(dst_name, args->lstio_tes_dgrp_name,
                           args->lstio_tes_dgrp_nmlen) ||
            copy_from_user(param, args->lstio_tes_param,
                              args->lstio_tes_param_len))
                goto out;
 
-        rc = lstcon_test_add(name,
-                            args->lstio_tes_type,
-                            args->lstio_tes_loop,
-                            args->lstio_tes_concur,
-                            args->lstio_tes_dist, args->lstio_tes_span,
-                            srcgrp, dstgrp, param, args->lstio_tes_param_len,
-                            &ret, args->lstio_tes_resultp);
+       rc = lstcon_test_add(batch_name,
+                           args->lstio_tes_type,
+                           args->lstio_tes_loop,
+                           args->lstio_tes_concur,
+                           args->lstio_tes_dist, args->lstio_tes_span,
+                           src_name, dst_name, param,
+                           args->lstio_tes_param_len,
+                           &ret, args->lstio_tes_resultp);
 
         if (ret != 0)
                rc = (copy_to_user(args->lstio_tes_retp, &ret,
                                        sizeof(ret))) ? -EFAULT : 0;
 out:
-        if (name != NULL)
-                LIBCFS_FREE(name, args->lstio_tes_bat_nmlen + 1);
+       if (batch_name != NULL)
+               LIBCFS_FREE(batch_name, args->lstio_tes_bat_nmlen + 1);
 
-        if (srcgrp != NULL)
-                LIBCFS_FREE(srcgrp, args->lstio_tes_sgrp_nmlen + 1);
+       if (src_name != NULL)
+               LIBCFS_FREE(src_name, args->lstio_tes_sgrp_nmlen + 1);
 
-        if (dstgrp != NULL)
-                LIBCFS_FREE(dstgrp, args->lstio_tes_dgrp_nmlen + 1);
+       if (dst_name != NULL)
+               LIBCFS_FREE(dst_name, args->lstio_tes_dgrp_nmlen + 1);
 
-        if (param != NULL)
-                LIBCFS_FREE(param, args->lstio_tes_param_len);
+       if (param != NULL)
+               LIBCFS_FREE(param, args->lstio_tes_param_len);
 
-        return rc;
+       return rc;
 }
 
 int
index 856caf7..27ad326 100644 (file)
@@ -287,13 +287,13 @@ lstcon_rpc_trans_addreq(lstcon_rpc_trans_t *trans, lstcon_rpc_t *crpc)
 void
 lstcon_rpc_trans_abort(lstcon_rpc_trans_t *trans, int error)
 {
-        srpc_client_rpc_t *rpc;
-        lstcon_rpc_t      *crpc;
-        lstcon_node_t     *nd;
+       srpc_client_rpc_t *rpc;
+       lstcon_rpc_t      *crpc;
+       lstcon_node_t     *nd;
 
-        cfs_list_for_each_entry_typed (crpc, &trans->tas_rpcs_list,
-                                       lstcon_rpc_t, crp_link) {
-                rpc = crpc->crp_rpc;
+       cfs_list_for_each_entry_typed(crpc, &trans->tas_rpcs_list,
+                                     lstcon_rpc_t, crp_link) {
+               rpc = crpc->crp_rpc;
 
                spin_lock(&rpc->crpc_lock);
 
@@ -312,18 +312,18 @@ lstcon_rpc_trans_abort(lstcon_rpc_trans_t *trans, int error)
 
                spin_unlock(&rpc->crpc_lock);
 
-                sfw_abort_rpc(rpc);
+               sfw_abort_rpc(rpc);
 
-                if  (error != ETIMEDOUT)
-                        continue;
+               if (error != -ETIMEDOUT)
+                       continue;
 
-                nd = crpc->crp_node;
-                if (cfs_time_after(nd->nd_stamp, crpc->crp_stamp))
-                        continue;
+               nd = crpc->crp_node;
+               if (cfs_time_after(nd->nd_stamp, crpc->crp_stamp))
+                       continue;
 
-                nd->nd_stamp = crpc->crp_stamp;
-                nd->nd_state = LST_NODE_DOWN;
-        }
+               nd->nd_stamp = crpc->crp_stamp;
+               nd->nd_state = LST_NODE_DOWN;
+       }
 }
 
 static int
index 9689506..d58cc9c 100644 (file)
@@ -269,21 +269,21 @@ lstcon_group_decref(lstcon_group_t *grp)
 }
 
 static int
-lstcon_group_find(char *name, lstcon_group_t **grpp)
+lstcon_group_find(const char *name, lstcon_group_t **grpp)
 {
-        lstcon_group_t   *grp;
+       lstcon_group_t   *grp;
 
-        cfs_list_for_each_entry_typed(grp, &console_session.ses_grp_list,
-                                      lstcon_group_t, grp_link) {
-                if (strncmp(grp->grp_name, name, LST_NAME_SIZE) != 0)
-                        continue;
+       cfs_list_for_each_entry_typed(grp, &console_session.ses_grp_list,
+                                     lstcon_group_t, grp_link) {
+               if (strncmp(grp->grp_name, name, LST_NAME_SIZE) != 0)
+                       continue;
 
-                lstcon_group_addref(grp);  /* +1 ref for caller */
-                *grpp = grp;
-                return 0;
-        }
+               lstcon_group_addref(grp);  /* +1 ref for caller */
+               *grpp = grp;
+               return 0;
+       }
 
-        return -ENOENT;
+       return -ENOENT;
 }
 
 static void
@@ -297,7 +297,7 @@ lstcon_group_ndlink_find(lstcon_group_t *grp, lnet_process_id_t id,
                          lstcon_ndlink_t **ndlpp, int create)
 {
         int     rc;
-        
+
         rc = lstcon_ndlink_find(&grp->grp_ndl_hash[0], id, ndlpp, create);
         if (rc != 0)
                 return rc;
@@ -837,20 +837,20 @@ lstcon_group_info(char *name, lstcon_ndlist_ent_t *gents_p,
         return 0;
 }
 
-int
-lstcon_batch_find(char *name, lstcon_batch_t **batpp)
+static int
+lstcon_batch_find(const char *name, lstcon_batch_t **batpp)
 {
-        lstcon_batch_t   *bat;
-
-        cfs_list_for_each_entry_typed(bat, &console_session.ses_bat_list,
-                                      lstcon_batch_t, bat_link) {
-                if (strncmp(bat->bat_name, name, LST_NAME_SIZE) == 0) {
-                        *batpp = bat;
-                        return 0;
-                }
-        }
+       lstcon_batch_t   *bat;
+
+       cfs_list_for_each_entry_typed(bat, &console_session.ses_bat_list,
+                                     lstcon_batch_t, bat_link) {
+               if (strncmp(bat->bat_name, name, LST_NAME_SIZE) == 0) {
+                       *batpp = bat;
+                       return 0;
+               }
+       }
 
-        return -ENOENT;
+       return -ENOENT;
 }
 
 int
@@ -1247,100 +1247,139 @@ again:
         goto again;
 }
 
-int
-lstcon_test_add(char *name, int type, int loop, int concur,
-                int dist, int span, char *src_name, char * dst_name,
-                void *param, int paramlen, int *retp,
-                cfs_list_t *result_up)
+static int
+lstcon_verify_batch(const char *name, lstcon_batch_t **batch)
 {
-        lstcon_group_t  *src_grp = NULL;
-        lstcon_group_t  *dst_grp = NULL;
-        lstcon_test_t   *test    = NULL;
-        lstcon_batch_t  *batch;
-        int              rc;
+       int rc;
 
-        rc = lstcon_batch_find(name, &batch);
-        if (rc != 0) {
-                CDEBUG(D_NET, "Can't find batch %s\n", name);
-                return rc;
-        }
+       rc = lstcon_batch_find(name, batch);
+       if (rc != 0) {
+               CDEBUG(D_NET, "Can't find batch %s\n", name);
+               return rc;
+       }
 
-        if (batch->bat_state != LST_BATCH_IDLE) {
-                CDEBUG(D_NET, "Can't change running batch %s\n", name);
-                return rc;
-        }
+       if ((*batch)->bat_state != LST_BATCH_IDLE) {
+               CDEBUG(D_NET, "Can't change running batch %s\n", name);
+               return -EINVAL;
+       }
 
-        rc = lstcon_group_find(src_name, &src_grp);
-        if (rc != 0) {
-                CDEBUG(D_NET, "Can't find group %s\n", src_name);
-                goto out;
-        }
+       return 0;
+}
 
-        rc = lstcon_group_find(dst_name, &dst_grp);
-        if (rc != 0) {
-                CDEBUG(D_NET, "Can't find group %s\n", dst_name);
-                goto out;
-        }
+static int
+lstcon_verify_group(const char *name, lstcon_group_t **grp)
+{
+       int                     rc;
+       lstcon_ndlink_t         *ndl;
 
-        if (dst_grp->grp_userland)
-                *retp = 1;
+       rc = lstcon_group_find(name, grp);
+       if (rc != 0) {
+               CDEBUG(D_NET, "can't find group %s\n", name);
+               return rc;
+       }
 
-        LIBCFS_ALLOC(test, offsetof(lstcon_test_t, tes_param[paramlen]));
-        if (!test) {
-                CERROR("Can't allocate test descriptor\n");
-                rc = -ENOMEM;
+       cfs_list_for_each_entry_typed(ndl, &(*grp)->grp_ndl_list,
+                                     lstcon_ndlink_t, ndl_link) {
+               if (ndl->ndl_node->nd_state == LST_NODE_ACTIVE) {
+                       return 0;
+               }
+       }
 
-                goto out;
-        }
+       CDEBUG(D_NET, "Group %s has no ACTIVE nodes\n", name);
 
-        memset(test, 0, offsetof(lstcon_test_t, tes_param[paramlen]));
-        test->tes_hdr.tsb_id    = batch->bat_hdr.tsb_id;
-        test->tes_batch         = batch;
-        test->tes_type          = type;
-        test->tes_oneside       = 0; /* TODO */
-        test->tes_loop          = loop;
-        test->tes_concur        = concur;
-        test->tes_stop_onerr    = 1; /* TODO */
-        test->tes_span          = span;
-        test->tes_dist          = dist;
-        test->tes_cliidx        = 0; /* just used for creating RPC */
-        test->tes_src_grp       = src_grp;
-        test->tes_dst_grp       = dst_grp;
-        CFS_INIT_LIST_HEAD(&test->tes_trans_list);
+       return -EINVAL;
+}
 
-        if (param != NULL) {
-                test->tes_paramlen = paramlen;
-                memcpy(&test->tes_param[0], param, paramlen);
-        }
+int
+lstcon_test_add(char *batch_name, int type, int loop,
+               int concur, int dist, int span,
+               char *src_name, char *dst_name,
+               void *param, int paramlen, int *retp,
+               cfs_list_t *result_up)
+{
+       lstcon_test_t    *test   = NULL;
+       int              rc;
+       lstcon_group_t   *src_grp = NULL;
+       lstcon_group_t   *dst_grp = NULL;
+       lstcon_batch_t   *batch = NULL;
+
+       /*
+        * verify that a batch of the given name exists, and the groups
+        * that will be part of the batch exist and have at least one
+        * active node
+        */
+       rc = lstcon_verify_batch(batch_name, &batch);
+       if (rc != 0)
+               goto out;
 
-        rc = lstcon_test_nodes_add(test, result_up);
+       rc = lstcon_verify_group(src_name, &src_grp);
+       if (rc != 0)
+               goto out;
 
-        if (rc != 0)
-                goto out;
+       rc = lstcon_verify_group(dst_name, &dst_grp);
+       if (rc != 0)
+               goto out;
 
-        if (lstcon_trans_stat()->trs_rpc_errno != 0 ||
-            lstcon_trans_stat()->trs_fwk_errno != 0)
-                CDEBUG(D_NET, "Failed to add test %d to batch %s\n", type, name);
+       if (dst_grp->grp_userland)
+               *retp = 1;
 
-        /* add to test list anyway, so user can check what's going on */
-        cfs_list_add_tail(&test->tes_link, &batch->bat_test_list);
+       LIBCFS_ALLOC(test, offsetof(lstcon_test_t, tes_param[paramlen]));
+       if (!test) {
+               CERROR("Can't allocate test descriptor\n");
+               rc = -ENOMEM;
 
-        batch->bat_ntest ++;
-        test->tes_hdr.tsb_index = batch->bat_ntest;
+               goto out;
+       }
 
-        /*  hold groups so nobody can change them */
-        return rc;
+       memset(test, 0, offsetof(lstcon_test_t, tes_param[paramlen]));
+       test->tes_hdr.tsb_id    = batch->bat_hdr.tsb_id;
+       test->tes_batch         = batch;
+       test->tes_type          = type;
+       test->tes_oneside       = 0; /* TODO */
+       test->tes_loop          = loop;
+       test->tes_concur        = concur;
+       test->tes_stop_onerr    = 1; /* TODO */
+       test->tes_span          = span;
+       test->tes_dist          = dist;
+       test->tes_cliidx        = 0; /* just used for creating RPC */
+       test->tes_src_grp       = src_grp;
+       test->tes_dst_grp       = dst_grp;
+       CFS_INIT_LIST_HEAD(&test->tes_trans_list);
+
+       if (param != NULL) {
+               test->tes_paramlen = paramlen;
+               memcpy(&test->tes_param[0], param, paramlen);
+       }
+
+       rc = lstcon_test_nodes_add(test, result_up);
+
+       if (rc != 0)
+               goto out;
+
+       if (lstcon_trans_stat()->trs_rpc_errno != 0 ||
+           lstcon_trans_stat()->trs_fwk_errno != 0)
+               CDEBUG(D_NET, "Failed to add test %d to batch %s\n", type,
+                      batch_name);
+
+       /* add to test list anyway, so user can check what's going on */
+       cfs_list_add_tail(&test->tes_link, &batch->bat_test_list);
+
+       batch->bat_ntest++;
+       test->tes_hdr.tsb_index = batch->bat_ntest;
+
+       /*  hold groups so nobody can change them */
+       return rc;
 out:
-        if (test != NULL)
-                LIBCFS_FREE(test, offsetof(lstcon_test_t, tes_param[paramlen]));
+       if (test != NULL)
+               LIBCFS_FREE(test, offsetof(lstcon_test_t, tes_param[paramlen]));
 
-        if (dst_grp != NULL)
-                lstcon_group_put(dst_grp);
+       if (dst_grp != NULL)
+               lstcon_group_put(dst_grp);
 
-        if (src_grp != NULL)
-                lstcon_group_put(src_grp);
+       if (src_grp != NULL)
+               lstcon_group_put(src_grp);
 
-        return rc;
+       return rc;
 }
 
 int
index 62f7208..0ddcc4d 100644 (file)
@@ -225,10 +225,11 @@ extern int lstcon_group_stat(char *grp_name, int timeout,
                              cfs_list_t *result_up);
 extern int lstcon_nodes_stat(int count, lnet_process_id_t *ids_up,
                              int timeout, cfs_list_t *result_up);
-extern int lstcon_test_add(char *name, int type, int loop, int concur,
-                           int dist, int span, char *src_name, char * dst_name,
-                           void *param, int paramlen, int *retp,
-                           cfs_list_t *result_up);
+extern int lstcon_test_add(char *batch_name, int type, int loop,
+                          int concur, int dist, int span,
+                          char *src_name, char *dst_name,
+                          void *param, int paramlen, int *retp,
+                          cfs_list_t *result_up);
 #endif
 
 #endif
index 34c3e8d..6ab41a9 100644 (file)
@@ -927,6 +927,54 @@ lst_add_nodes_ioctl (char *name, int count, lnet_process_id_t *ids,
 }
 
 int
+lst_del_group_ioctl(char *name)
+{
+       lstio_group_del_args_t args = {0};
+
+       args.lstio_grp_key   = session_key;
+       args.lstio_grp_nmlen = strlen(name);
+       args.lstio_grp_namep = name;
+
+       return lst_ioctl(LSTIO_GROUP_DEL, &args, sizeof(args));
+}
+
+int
+lst_del_group(char *grp_name)
+{
+       int     rc;
+
+       rc = lst_del_group_ioctl(grp_name);
+       if (rc == 0) {
+               fprintf(stdout, "Group is deleted\n");
+               return 0;
+       }
+
+       if (rc == -1) {
+               lst_print_error("group", "Failed to delete group: %s\n",
+                               strerror(errno));
+               return rc;
+       }
+
+       fprintf(stderr, "Group is deleted with some errors\n");
+
+       if (trans_stat.trs_rpc_errno != 0) {
+               fprintf(stderr,
+                       "[RPC] Failed to send %d end session RPCs: %s\n",
+                       lstcon_rpc_stat_failure(&trans_stat, 0),
+                       strerror(trans_stat.trs_rpc_errno));
+       }
+
+       if (trans_stat.trs_fwk_errno != 0) {
+               fprintf(stderr,
+                       "[FWK] Failed to end session on %d nodes: %s\n",
+               lstcon_sesop_stat_failure(&trans_stat, 0),
+               strerror(trans_stat.trs_fwk_errno));
+       }
+
+       return -1;
+}
+
+int
 lst_add_group_ioctl (char *name)
 {
         lstio_group_add_args_t args = {0};
@@ -941,57 +989,60 @@ lst_add_group_ioctl (char *name)
 int
 jt_lst_add_group(int argc, char **argv)
 {
-        cfs_list_t         head;
-        lnet_process_id_t *ids;
-        char              *name;
+       cfs_list_t         head;
+       lnet_process_id_t *ids;
+       char              *name;
        unsigned           feats = session_features;
-        int                count;
-        int                rc;
-        int                i;
+       int                count;
+       int                rc;
+       int                i;
+       bool               nodes_added = false;
 
-        if (session_key == 0) {
-                fprintf(stderr,
-                        "Can't find env LST_SESSION or value is not valid\n");
-                return -1;
-        }
+       if (session_key == 0) {
+               fprintf(stderr,
+                       "Can't find env LST_SESSION or value is not valid\n");
+               return -1;
+       }
 
-        if (argc < 3) {
-                lst_print_usage(argv[0]);
-                return -1;
-        }
+       if (argc < 3) {
+               lst_print_usage(argv[0]);
+               return -1;
+       }
 
-        name = argv[1];
-        if (strlen(name) >= LST_NAME_SIZE) {
-                fprintf(stderr, "Name length is limited to %d\n",
-                        LST_NAME_SIZE - 1);
-                return -1;
-        }
+       name = argv[1];
+       if (strlen(name) >= LST_NAME_SIZE) {
+               fprintf(stderr, "Name length is limited to %d\n",
+                       LST_NAME_SIZE - 1);
+               return -1;
+       }
 
-        rc = lst_add_group_ioctl(name);
-        if (rc != 0) {
-                lst_print_error("group", "Failed to add group %s: %s\n",
-                                name, strerror(errno));
-                return -1;
-        }
+       rc = lst_add_group_ioctl(name);
+       if (rc != 0) {
+               lst_print_error("group", "Failed to add group %s: %s\n",
+                               name, strerror(errno));
+               return -1;
+       }
 
-        CFS_INIT_LIST_HEAD(&head);
+       CFS_INIT_LIST_HEAD(&head);
 
-        for (i = 2; i < argc; i++) {
-                /* parse address list */
-                rc = lst_parse_nids(argv[i], &count, &ids);
-                if (rc < 0) {
-                        fprintf(stderr, "Ignore invalid id list %s\n",
-                                argv[i]);
-                        continue;
-                }
+       for (i = 2; i < argc; i++) {
+               /* parse address list */
+               rc = lst_parse_nids(argv[i], &count, &ids);
+               if (rc < 0) {
+                       fprintf(stderr, "Ignore invalid id list %s\n",
+                               argv[i]);
+                       continue;
+               }
 
-                if (count == 0)
-                        continue;
+               if (count == 0)
+                       continue;
 
-                rc = lst_alloc_rpcent(&head, count, 0);
-                if (rc != 0) {
-                        fprintf(stderr, "Out of memory\n");
-                       return -1;
+               rc = lst_alloc_rpcent(&head, count, 0);
+               if (rc != 0) {
+                       fprintf(stderr, "Out of memory\n");
+                       free(ids);
+                       rc = -1;
+                       goto failed;
                }
 
                rc = lst_add_nodes_ioctl(name, count, ids, &feats, &head);
@@ -1003,6 +1054,8 @@ jt_lst_add_group(int argc, char **argv)
 
                fprintf(stdout, "%s are added to session\n", argv[i]);
 
+               nodes_added = true;
+
                if ((feats & session_features) != session_features) {
                        fprintf(stdout,
                                "Warning, this session will run with "
@@ -1014,7 +1067,24 @@ jt_lst_add_group(int argc, char **argv)
                lst_free_rpcent(&head);
        }
 
-       return 0;
+       if (!nodes_added) {
+               /*
+                * The selftest kernel module expects that a group should
+                * have at least one node, since it doesn't make sense for
+                * an empty group to be added to a test.
+                */
+               fprintf(stderr,
+                       "No nodes added successfully, deleting group %s\n",
+                       name);
+               rc = lst_del_group(name);
+               if (rc != 0) {
+                       fprintf(stderr,
+                               "Failed to delete group %s."
+                               "  Group is empty.\n", name);
+               }
+       }
+
+       return rc;
 
 failed:
        if (rc == -1) {
@@ -1034,19 +1104,18 @@ failed:
 
        lst_free_rpcent(&head);
 
-       return rc;
-}
-
-int
-lst_del_group_ioctl (char *name)
-{
-        lstio_group_del_args_t args = {0};
-
-        args.lstio_grp_key   = session_key;
-        args.lstio_grp_nmlen = strlen(name);
-        args.lstio_grp_namep = name;
+       if (!nodes_added) {
+               fprintf(stderr,
+                       "No nodes added successfully, deleting group %s\n",
+                       name);
+               if (lst_del_group(name) != 0) {
+                       fprintf(stderr,
+                               "Failed to delete group %s."
+                               "  Group is empty.\n", name);
+               }
+       }
 
-        return lst_ioctl(LSTIO_GROUP_DEL, &args, sizeof(args));
+       return rc;
 }
 
 int
@@ -1065,34 +1134,9 @@ jt_lst_del_group(int argc, char **argv)
                 return -1;
         }
 
-        rc = lst_del_group_ioctl(argv[1]);
-        if (rc == 0) {
-                fprintf(stdout, "Group is deleted\n");
-                return 0;
-        }
-
-        if (rc == -1) {
-                lst_print_error("group", "Failed to delete group: %s\n",
-                                strerror(errno));
-                return rc;
-        }
-
-        fprintf(stderr, "Group is deleted with some errors\n");
-
-        if (trans_stat.trs_rpc_errno != 0) {
-                fprintf(stderr, "[RPC] Failed to send %d end session RPCs: %s\n",
-                        lstcon_rpc_stat_failure(&trans_stat, 0),
-                        strerror(trans_stat.trs_rpc_errno));
-        }
-
-        if (trans_stat.trs_fwk_errno != 0) {
-                fprintf(stderr,
-                        "[FWK] Failed to end session on %d nodes: %s\n",
-                        lstcon_sesop_stat_failure(&trans_stat, 0),
-                        strerror(trans_stat.trs_fwk_errno));
-        }
+       rc = lst_del_group(argv[1]);
 
-        return -1;
+       return rc;
 }
 
 int