Whamcloud - gitweb
LU-9680 lnet: handle multi-rail setups 26/50026/5
authorJames Simmons <jsimmons@infradead.org>
Tue, 7 Mar 2023 17:44:38 +0000 (12:44 -0500)
committerOleg Drokin <green@whamcloud.com>
Tue, 28 Mar 2023 22:09:38 +0000 (22:09 +0000)
For multi-rail setups we can push more than one interface at a
time to setup the local NIs but our netlink code ignored all but
one interface. Refactor both lnet_genl_parse_local_ni() and
lnet_net_cmd() to setup all the passed in interfaces. Also remove
setting ni to NULL in the NI deletion case which causes an oops
when we have more than one interface.

Lastly rework the Netlink userland library code to properly pack
netlink packets sent to the kernel. We were treating YAMl mappings
the same as YAML sequences. This is wrong so we separate the
handling of each case. Mapping then are translated as nested
collection of data and sequences are arrays of these data. This
ends up packing a nested collection in another nested collection.
Before we didn't have this layering which lead to improper
packing.

Test-Parameters: trivial testlist=sanity-lnet
Change-Id: Icb220127fdabfc5ebf4bb848cf2715048c40f674
Signed-off-by: James Simmons <jsimmons@infradead.org>
Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/50026
Tested-by: Maloo <maloo@whamcloud.com>
Tested-by: jenkins <devops@whamcloud.com>
Reviewed-by: Serguei Smirnov <ssmirnov@whamcloud.com>
Reviewed-by: Chris Horn <chris.horn@hpe.com>
Reviewed-by: Cyril Bordage <cbordage@whamcloud.com>
Reviewed-by: Frank Sehr <fsehr@whamcloud.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>
lnet/lnet/api-ni.c
lnet/selftest/conctl.c
lnet/utils/lnetconfig/liblnetconfig_netlink.c
lustre/obdclass/kernelcomm.c

index 4d95812..c0f9293 100644 (file)
@@ -4774,7 +4774,12 @@ static int lnet_net_show_start(struct netlink_callback *cb)
                return 0;
 
        params = genlmsg_data(gnlh);
-       nla_for_each_attr(top, params, msg_len, rem) {
+       if (!(nla_type(params) & LN_SCALAR_ATTR_LIST)) {
+               NL_SET_ERR_MSG(extack, "invalid configuration");
+               return -EINVAL;
+       }
+
+       nla_for_each_nested(top, params, rem) {
                struct nlattr *net;
                int rem2;
 
@@ -4782,7 +4787,7 @@ static int lnet_net_show_start(struct netlink_callback *cb)
                        char filter[LNET_NIDSTR_SIZE];
 
                        if (nla_type(net) != LN_SCALAR_ATTR_VALUE ||
-                           nla_strcmp(net, "name") != 0)
+                           nla_strcmp(net, "net type") != 0)
                                continue;
 
                        net = nla_next(net, &rem2);
@@ -5027,12 +5032,21 @@ static int lnet_genl_parse_lnd_tunables(struct nlattr *settings,
 static int
 lnet_genl_parse_local_ni(struct nlattr *entry, struct genl_info *info,
                         int net_id, struct lnet_ioctl_config_ni *conf,
-                        struct lnet_ioctl_config_lnd_tunables *tun,
                         bool *ni_list)
 {
+       bool create = info->nlhdr->nlmsg_flags & NLM_F_CREATE;
+       struct lnet_ioctl_config_lnd_tunables tun;
        struct nlattr *settings;
        int rem3, rc = 0;
 
+       memset(&tun, 0, sizeof(tun));
+       /* Use LND defaults */
+       tun.lt_cmn.lct_peer_timeout = -1;
+       tun.lt_cmn.lct_peer_tx_credits = -1;
+       tun.lt_cmn.lct_peer_rtr_credits = -1;
+       tun.lt_cmn.lct_max_tx_credits = -1;
+       conf->lic_ncpts = 0;
+
        nla_for_each_nested(settings, entry, rem3) {
                if (nla_type(settings) != LN_SCALAR_ATTR_VALUE)
                        continue;
@@ -5076,7 +5090,7 @@ lnet_genl_parse_local_ni(struct nlattr *entry, struct genl_info *info,
                                GOTO(out, rc = -EINVAL);
                        }
 
-                       rc = lnet_genl_parse_tunables(settings, tun);
+                       rc = lnet_genl_parse_tunables(settings, &tun);
                        if (rc < 0) {
                                GENL_SET_ERR_MSG(info,
                                                 "failed to parse tunables");
@@ -5101,7 +5115,7 @@ lnet_genl_parse_local_ni(struct nlattr *entry, struct genl_info *info,
                        }
 
                        rc = lnet_genl_parse_lnd_tunables(settings,
-                                                         &tun->lt_tun, lnd);
+                                                         &tun.lt_tun, lnd);
                        if (rc < 0) {
                                GENL_SET_ERR_MSG(info,
                                                 "failed to parse lnd tunables");
@@ -5140,6 +5154,73 @@ lnet_genl_parse_local_ni(struct nlattr *entry, struct genl_info *info,
                        }
                }
        }
+
+       if (!create) {
+               struct lnet_net *net;
+               struct lnet_ni *ni;
+
+               rc = -ENODEV;
+               if (!strlen(conf->lic_ni_intf)) {
+                       GENL_SET_ERR_MSG(info,
+                                        "interface is missing");
+                       GOTO(out, rc);
+               }
+
+               lnet_net_lock(LNET_LOCK_EX);
+               net = lnet_get_net_locked(net_id);
+               if (!net) {
+                       GENL_SET_ERR_MSG(info,
+                                        "LNet net doesn't exist");
+                       lnet_net_unlock(LNET_LOCK_EX);
+                       GOTO(out, rc);
+               }
+
+               list_for_each_entry(ni, &net->net_ni_list,
+                                   ni_netlist) {
+                       if (!ni->ni_interface ||
+                           strcmp(ni->ni_interface,
+                                 conf->lic_ni_intf) != 0)
+                               continue;
+
+                       lnet_net_unlock(LNET_LOCK_EX);
+                       rc = lnet_dyn_del_ni(&ni->ni_nid);
+                       if (rc < 0) {
+                               GENL_SET_ERR_MSG(info,
+                                                "cannot del LNet NI");
+                               GOTO(out, rc);
+                       }
+                       break;
+               }
+
+               if (rc < 0) { /* will be -ENODEV */
+                       GENL_SET_ERR_MSG(info,
+                                        "interface invalid for deleting LNet NI");
+                       lnet_net_unlock(LNET_LOCK_EX);
+               }
+       } else {
+               if (!strlen(conf->lic_ni_intf)) {
+                       GENL_SET_ERR_MSG(info,
+                                        "interface is missing");
+                       GOTO(out, rc);
+               }
+
+               rc = lnet_dyn_add_ni(conf, net_id, &tun);
+               switch (rc) {
+               case -ENOENT:
+                       GENL_SET_ERR_MSG(info,
+                                        "cannot parse net");
+                       break;
+               case -ERANGE:
+                       GENL_SET_ERR_MSG(info,
+                                        "invalid CPT set");
+                       break;
+               default:
+                       GENL_SET_ERR_MSG(info,
+                                        "cannot add LNet NI");
+               case 0:
+                       break;
+               }
+       }
 out:
        return rc;
 }
@@ -5158,7 +5239,12 @@ static int lnet_net_cmd(struct sk_buff *skb, struct genl_info *info)
                return -ENOMSG;
        }
 
-       nla_for_each_attr(attr, params, msg_len, rem) {
+       if (!(nla_type(params) & LN_SCALAR_ATTR_LIST)) {
+               GENL_SET_ERR_MSG(info, "invalid configuration");
+               return -EINVAL;
+       }
+
+       nla_for_each_nested(attr, params, rem) {
                struct lnet_ioctl_config_ni conf;
                u32 net_id = LNET_NET_ANY;
                struct nlattr *entry;
@@ -5231,85 +5317,13 @@ static int lnet_net_cmd(struct sk_buff *skb, struct genl_info *info)
                                break;
                        }
                        case LN_SCALAR_ATTR_LIST: {
-                               bool create = info->nlhdr->nlmsg_flags &
-                                             NLM_F_CREATE;
-                               struct lnet_ioctl_config_lnd_tunables tun;
-
-                               memset(&tun, 0, sizeof(tun));
-                               /* Use LND defaults */
-                               tun.lt_cmn.lct_peer_timeout = -1;
-                               tun.lt_cmn.lct_peer_tx_credits = -1;
-                               tun.lt_cmn.lct_peer_rtr_credits = -1;
-                               tun.lt_cmn.lct_max_tx_credits = -1;
-                               conf.lic_ncpts = 0;
-
-                               rc = lnet_genl_parse_local_ni(entry, info,
-                                                             net_id, &conf,
-                                                             &tun, &ni_list);
-                               if (rc < 0)
-                                       GOTO(out, rc);
+                               struct nlattr *interface;
+                               int rem3;
 
-                               if (!create) {
-                                       struct lnet_net *net;
-                                       struct lnet_ni *ni;
-
-                                       rc = -ENODEV;
-                                       if (!strlen(conf.lic_ni_intf)) {
-                                               GENL_SET_ERR_MSG(info,
-                                                                "interface is missing");
-                                               GOTO(out, rc);
-                                       }
-
-                                       lnet_net_lock(LNET_LOCK_EX);
-                                       net = lnet_get_net_locked(net_id);
-                                       if (!net) {
-                                               GENL_SET_ERR_MSG(info,
-                                                                "LNet net doesn't exist");
-                                               lnet_net_unlock(LNET_LOCK_EX);
-                                               GOTO(out, rc);
-                                       }
-                                       list_for_each_entry(ni, &net->net_ni_list,
-                                                           ni_netlist) {
-                                               if (!ni->ni_interface ||
-                                                   strncmp(ni->ni_interface,
-                                                           conf.lic_ni_intf,
-                                                           strlen(conf.lic_ni_intf)) != 0) {
-                                                       ni = NULL;
-                                                       continue;
-                                               }
-
-                                               lnet_net_unlock(LNET_LOCK_EX);
-                                               rc = lnet_dyn_del_ni(&ni->ni_nid);
-                                               if (rc < 0) {
-                                                       GENL_SET_ERR_MSG(info,
-                                                                        "cannot del LNet NI");
-                                                       GOTO(out, rc);
-                                               }
-                                               break;
-                                       }
-
-                                       if (rc < 0) { /* will be -ENODEV */
-                                               GENL_SET_ERR_MSG(info,
-                                                                "interface invalid for deleting LNet NI");
-                                               lnet_net_unlock(LNET_LOCK_EX);
-                                       }
-                               } else {
-                                       rc = lnet_dyn_add_ni(&conf, net_id, &tun);
-                                       switch (rc) {
-                                       case -ENOENT:
-                                               GENL_SET_ERR_MSG(info,
-                                                                "cannot parse net");
-                                               break;
-                                       case -ERANGE:
-                                               GENL_SET_ERR_MSG(info,
-                                                                "invalid CPT set");
-                                       fallthrough;
-                                       default:
-                                               GENL_SET_ERR_MSG(info,
-                                                                "cannot add LNet NI");
-                                       case 0:
-                                               break;
-                                       }
+                               nla_for_each_nested(interface, entry, rem3) {
+                                       rc = lnet_genl_parse_local_ni(interface, info,
+                                                                     net_id, &conf,
+                                                                     &ni_list);
                                        if (rc < 0)
                                                GOTO(out, rc);
                                }
@@ -5695,6 +5709,7 @@ static const struct genl_multicast_group lnet_mcast_grps[] = {
 static const struct genl_ops lnet_genl_ops[] = {
        {
                .cmd            = LNET_CMD_NETS,
+               .flags          = GENL_ADMIN_PERM,
 #ifdef HAVE_NETLINK_CALLBACK_START
                .start          = lnet_net_show_start,
                .dumpit         = lnet_net_show_dump,
index ff13527..fef72dc 100644 (file)
@@ -1139,7 +1139,13 @@ static int lst_groups_show_start(struct netlink_callback *cb)
 #ifdef HAVE_NL_DUMP_WITH_EXT_ACK
        extack = cb->extack;
 #endif
-       nla_for_each_attr(groups, params, msg_len, rem) {
+
+       if (!(nla_type(params) & LN_SCALAR_ATTR_LIST)) {
+               NL_SET_ERR_MSG(extack, "no configuration");
+               GOTO(report_err, rc);
+       }
+
+       nla_for_each_nested(groups, params, rem) {
                struct lst_genl_group_prop *prop = NULL;
                struct nlattr *group;
                int rem2;
index f5e4126..17ab2d4 100644 (file)
@@ -567,14 +567,14 @@ static void yaml_parse_value_list(struct yaml_netlink_input *data, int *size,
                case NLA_NESTED: {
                        struct yaml_nl_node *next = get_next_child(node,
                                                                   child_idx++);
-                       int num = next->keys.lkl_maxattr;
+                       int num = next ? next->keys.lkl_maxattr : 0;
                        struct nla_policy nest_policy[num];
                        struct yaml_nl_node *old;
                        struct nlattr *cnt_attr;
                        uint16_t indent = 0;
                        int rem, j;
 
-                       if (!attr)
+                       if (!attr || !next)
                                continue;
 
                        memset(nest_policy, 0, sizeof(struct nla_policy) * num);
@@ -1081,7 +1081,7 @@ static unsigned int indent_level(const char *str)
        return tmp - str;
 }
 
-#define LNKF_END 8
+#define LNKF_BLOCK 8
 
 static enum lnet_nl_key_format yaml_format_type(yaml_emitter_t *emitter,
                                                char *line,
@@ -1094,12 +1094,13 @@ static enum lnet_nl_key_format yaml_format_type(yaml_emitter_t *emitter,
        new_indent = indent_level(line);
        if (new_indent < indent) {
                *offset = indent - emitter->best_indent;
-               return LNKF_END;
+               return LNKF_BLOCK;
        }
 
        if (strncmp(line + new_indent, "- ", 2) == 0) {
                memset(line + new_indent, ' ', 2);
                new_indent += 2;
+               fmt |= LNKF_SEQUENCE;
        }
 
        /* hdr: [ a : 1, b : 2, c : 3 ] */
@@ -1124,7 +1125,7 @@ static enum lnet_nl_key_format yaml_format_type(yaml_emitter_t *emitter,
 
        if (indent != new_indent) {
                *offset = new_indent;
-               fmt |= LNKF_SEQUENCE;
+               fmt |= LNKF_BLOCK;
        }
 
        return fmt;
@@ -1183,15 +1184,14 @@ static int yaml_create_nested_list(struct yaml_netlink_output *out,
                                   char **entry, unsigned int *indent,
                                   enum lnet_nl_key_format fmt)
 {
-       bool nested = fmt & LNKF_SEQUENCE;
-       struct nlattr *list = NULL;
-       char *line;
+       struct nlattr *mapping = NULL, *seq = NULL;
+       char *line, *tmp;
        int rc = 0;
 
        /* Not needed for FLOW only case */
-       if (nested) {
-               list = nla_nest_start(msg, LN_SCALAR_ATTR_LIST);
-               if (!list) {
+       if (fmt & LNKF_SEQUENCE) {
+               seq = nla_nest_start(msg, LN_SCALAR_ATTR_LIST);
+               if (!seq) {
                        yaml_emitter_set_writer_error(out->emitter,
                                                      "Emmitter netlink list creation failed");
                        rc = -EINVAL;
@@ -1199,16 +1199,18 @@ static int yaml_create_nested_list(struct yaml_netlink_output *out,
                }
        }
 
-       if (fmt != LNKF_FLOW) {
-               rc = yaml_fill_scalar_data(msg, fmt, *hdr + *indent);
-               if (rc < 0)
-                       goto nla_put_failure;
-       }
-
        if (fmt & LNKF_FLOW) {
-               char *tmp = strchr(*hdr, '{'), *split = NULL;
+               struct nlattr *list = NULL;
                bool format = false;
+               char *split = NULL;
+
+               if (fmt != LNKF_FLOW) {
+                       rc = yaml_fill_scalar_data(msg, fmt, *hdr + *indent);
+                       if (rc < 0)
+                               goto nla_put_failure;
+               }
 
+               tmp = strchr(*hdr, '{');
                if (!tmp) {
                        tmp = strchr(*hdr, '[');
                        if (!tmp) {
@@ -1276,6 +1278,21 @@ static int yaml_create_nested_list(struct yaml_netlink_output *out,
 
                nla_nest_end(msg, list);
        } else {
+next_mapping:
+               if (fmt & LNKF_BLOCK && strchr(*hdr, ':')) {
+                       mapping = nla_nest_start(msg, LN_SCALAR_ATTR_LIST);
+                       if (!mapping) {
+                               yaml_emitter_set_writer_error(out->emitter,
+                                                             "Emmitter netlink list creation failed");
+                               rc = -EINVAL;
+                               goto nla_put_failure;
+                       }
+               }
+
+               rc = yaml_fill_scalar_data(msg, fmt, *hdr + *indent);
+               if (rc < 0)
+                       goto nla_put_failure;
+
                do {
                        line = strsep(entry, "\n");
 have_next_line:
@@ -1283,41 +1300,88 @@ have_next_line:
                                break;
 
                        fmt = yaml_format_type(out->emitter, line, indent);
-                       if (fmt == LNKF_END)
+                       if (fmt == LNKF_BLOCK)
                                break;
 
-                       if (fmt & ~LNKF_MAPPING) { /* Filter out mappings */
+                       /* sequences of simple scalars, general mappings, and
+                        * plain scalars are not nested structures in a
+                        * netlink packet.
+                        */
+                       if (fmt == LNKF_SEQUENCE || fmt == LNKF_MAPPING || fmt == 0) {
+                               rc = yaml_fill_scalar_data(msg, fmt,
+                                                          line + *indent);
+                               if (rc < 0)
+                                       goto nla_put_failure;
+                       } else {
                                rc = yaml_create_nested_list(out, msg, &line,
                                                             entry, indent,
                                                             fmt);
                                if (rc < 0)
                                        goto nla_put_failure;
+
+                               /* if the original line that called
+                                * yaml_create_nested_list above was an
+                                * sequence and the next line is also
+                                * then break to treat it as a mapping / scalar
+                                * instead to avoid over nesting.
+                                */
+                               if (line && seq) {
+                                       fmt = yaml_format_type(out->emitter, line, indent);
+                                       if ((fmt & LNKF_SEQUENCE) || (fmt & LNKF_BLOCK))
+                                               break;
+                               }
+
                                if (line)
                                        goto have_next_line;
-                       } else {
-                               rc = yaml_fill_scalar_data(msg, fmt,
-                                                          line + *indent);
-                               if (rc < 0)
-                                       goto nla_put_failure;
                        }
                } while (strcmp(*entry, ""));
 
-               if (line && line[*indent] == '-') {
-                       line[*indent] = ' ';
-                       *indent += 2;
+               if (mapping) {
+                       nla_nest_end(msg, mapping);
+                       mapping = NULL;
+               }
+       }
+
+       /* test if next line is sequence at the same level. */
+       if (line && (line[0] != '\0') && (fmt & LNKF_BLOCK)) {
+               int old_indent = indent_level(*hdr);
+
+               fmt = yaml_format_type(out->emitter, line, indent);
+               if (fmt != LNKF_BLOCK && old_indent == *indent) {
+                       /* If we have a normal mapping set then treate
+                        * it as a collection of scalars i.e don't create
+                        * another nested level. For scalar:\n and plain
+                        * scalar case we send it to next_mapping to
+                        * create another nested level.
+                        */
+                       tmp = strchr(line, ':');
+                       if (tmp) {
+                               fmt = LNKF_BLOCK;
+                               if (strstr(line, ": "))
+                                       fmt |= LNKF_MAPPING;
+                               if (strstr(line, "- "))
+                                       fmt |= LNKF_SEQUENCE;
+                               *hdr = line;
+                               goto next_mapping;
+                       }
+
                        goto have_next_line;
                }
-               if (*entry && !strlen(*entry))
+       }
+
+       if (seq) {
+               if (*indent >= 2)
+                       *indent -= 2;
+               nla_nest_end(msg, seq);
+               seq = NULL;
+               if (*entry && !strlen(*entry) && fmt != LNKF_BLOCK)
                        line = NULL;
-               /* strsep in the above loop moves entry to a value pass the
-                * end of the nested list. So to avoid losing this value we
-                * replace hdr with line.
-                */
-               *hdr = line;
        }
 
-       if (nested)
-               nla_nest_end(msg, list);
+       /* strsep in the above loop moves entry to a value pass the end of the
+        * nested list. So to avoid losing this value we replace hdr with line.
+        */
+       *hdr = line;
 nla_put_failure:
        return rc;
 }
@@ -1430,7 +1494,7 @@ already_have_line:
                        }
 
                        fmt = yaml_format_type(out->emitter, line, &indent);
-                       if (fmt & ~LNKF_MAPPING) {
+                       if (fmt) {
                                rc = yaml_create_nested_list(out, msg, &line,
                                                             &entry, &indent,
                                                             fmt);
index 98c21e8..8407825 100644 (file)
@@ -121,7 +121,12 @@ static int lustre_device_list_start(struct netlink_callback *cb)
                struct nlattr *dev;
                int rem;
 
-               nla_for_each_attr(dev, params, msg_len, rem) {
+               if (!(nla_type(params) & LN_SCALAR_ATTR_LIST)) {
+                       NL_SET_ERR_MSG(extack, "no configuration");
+                       GOTO(report_err, rc);
+               }
+
+               nla_for_each_nested(dev, params, rem) {
                        struct nlattr *prop;
                        int rem2;