| Message ID | 20231019082138.18889-3-phaddad@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support to set privileged qkey parameter | expand |
Patrisious Haddad <phaddad@nvidia.com> writes: > @@ -40,6 +45,22 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data) > mode_str); > } > > + if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) { > + const char *pqkey_str; > + uint8_t pqkey_mode; > + > + pqkey_mode = > + mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]); > + > + if (pqkey_mode < ARRAY_SIZE(privileged_qkey_str)) > + pqkey_str = privileged_qkey_str[pqkey_mode]; > + else > + pqkey_str = "unknown"; > + > + print_color_string(PRINT_ANY, COLOR_NONE, "privileged-qkey", > + "privileged-qkey %s ", pqkey_str); > + } > + Elsewhere in the file, you just use print_color_on_off(), why not here? > if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]) > cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]); > > @@ -111,10 +155,25 @@ static int sys_set_netns_args(struct rd *rd) > return sys_set_netns_cmd(rd, cmd); > } > > +static int sys_set_privileged_qkey_args(struct rd *rd) > +{ > + bool cmd; > + > + if (rd_no_arg(rd) || !sys_valid_privileged_qkey_cmd(rd_argv(rd))) { > + pr_err("valid options are: { on | off }\n"); > + return -EINVAL; > + } This could use parse_on_off(). > + > + cmd = (strcmp(rd_argv(rd), "on") == 0) ? true : false; > + > + return sys_set_privileged_qkey_cmd(rd, cmd); > +} > + > static int sys_set_help(struct rd *rd) > { > pr_out("Usage: %s system set [PARAM] value\n", rd->filename); > pr_out(" system set netns { shared | exclusive }\n"); > + pr_out(" system set privileged-qkey { on | off }\n"); > return 0; > } > > @@ -124,6 +183,7 @@ static int sys_set(struct rd *rd) > { NULL, sys_set_help }, > { "help", sys_set_help }, > { "netns", sys_set_netns_args}, > + { "privileged-qkey", sys_set_privileged_qkey_args}, > { 0 } > }; The rest of the code looks sane to me, but I'm not familiar with the feature.
On 10/19/23 4:38 AM, Petr Machata wrote: > > Patrisious Haddad <phaddad@nvidia.com> writes: > >> @@ -40,6 +45,22 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data) >> mode_str); >> } >> >> + if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) { >> + const char *pqkey_str; >> + uint8_t pqkey_mode; >> + >> + pqkey_mode = >> + mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]); >> + >> + if (pqkey_mode < ARRAY_SIZE(privileged_qkey_str)) >> + pqkey_str = privileged_qkey_str[pqkey_mode]; >> + else >> + pqkey_str = "unknown"; >> + >> + print_color_string(PRINT_ANY, COLOR_NONE, "privileged-qkey", >> + "privileged-qkey %s ", pqkey_str); >> + } >> + > > Elsewhere in the file, you just use print_color_on_off(), why not here? +1
On 10/19/2023 1:38 PM, Petr Machata wrote: > Patrisious Haddad <phaddad@nvidia.com> writes: > >> @@ -40,6 +45,22 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data) >> mode_str); >> } >> >> + if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) { >> + const char *pqkey_str; >> + uint8_t pqkey_mode; >> + >> + pqkey_mode = >> + mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]); >> + >> + if (pqkey_mode < ARRAY_SIZE(privileged_qkey_str)) >> + pqkey_str = privileged_qkey_str[pqkey_mode]; >> + else >> + pqkey_str = "unknown"; >> + >> + print_color_string(PRINT_ANY, COLOR_NONE, "privileged-qkey", >> + "privileged-qkey %s ", pqkey_str); >> + } >> + > Elsewhere in the file, you just use print_color_on_off(), why not here? The print_color_on_off was used for copy-on-fork which as you see has no set function, I was simply trying to be consistent with this file convention & style, whereas print_color_string was used for the other configurable value ("netns"), I can obviously change that if you all see it as necessary. > >> if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]) >> cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]); >> >> @@ -111,10 +155,25 @@ static int sys_set_netns_args(struct rd *rd) >> return sys_set_netns_cmd(rd, cmd); >> } >> >> +static int sys_set_privileged_qkey_args(struct rd *rd) >> +{ >> + bool cmd; >> + >> + if (rd_no_arg(rd) || !sys_valid_privileged_qkey_cmd(rd_argv(rd))) { >> + pr_err("valid options are: { on | off }\n"); >> + return -EINVAL; >> + } > This could use parse_on_off(). You are absolutely correct, but just as well was trying to maintain same code style as the previous configurable value we have here, but I think using parse_on_off here can save us some code. > >> + >> + cmd = (strcmp(rd_argv(rd), "on") == 0) ? true : false; >> + >> + return sys_set_privileged_qkey_cmd(rd, cmd); >> +} >> + >> static int sys_set_help(struct rd *rd) >> { >> pr_out("Usage: %s system set [PARAM] value\n", rd->filename); >> pr_out(" system set netns { shared | exclusive }\n"); >> + pr_out(" system set privileged-qkey { on | off }\n"); >> return 0; >> } >> >> @@ -124,6 +183,7 @@ static int sys_set(struct rd *rd) >> { NULL, sys_set_help }, >> { "help", sys_set_help }, >> { "netns", sys_set_netns_args}, >> + { "privileged-qkey", sys_set_privileged_qkey_args}, >> { 0 } >> }; > The rest of the code looks sane to me, but I'm not familiar with the > feature. If no one else has any comments soon, and these two comments are actually considered critical I can re-send my patches with those issues fixed.
On 10/19/2023 1:38 PM, Petr Machata wrote: > Patrisious Haddad <phaddad@nvidia.com> writes: > >> @@ -40,6 +45,22 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data) >> mode_str); >> } >> >> + if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) { >> + const char *pqkey_str; >> + uint8_t pqkey_mode; >> + >> + pqkey_mode = >> + mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]); >> + >> + if (pqkey_mode < ARRAY_SIZE(privileged_qkey_str)) >> + pqkey_str = privileged_qkey_str[pqkey_mode]; >> + else >> + pqkey_str = "unknown"; >> + >> + print_color_string(PRINT_ANY, COLOR_NONE, "privileged-qkey", >> + "privileged-qkey %s ", pqkey_str); >> + } >> + > Elsewhere in the file, you just use print_color_on_off(), why not here? About this as I previously answered I don't really see a big difference between it and "print_color_string" but if the maintainer thinks this is an essential change I can fix and re-send. Thanks for the review. > >> if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]) >> cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]); >> >> @@ -111,10 +155,25 @@ static int sys_set_netns_args(struct rd *rd) >> return sys_set_netns_cmd(rd, cmd); >> } >> >> +static int sys_set_privileged_qkey_args(struct rd *rd) >> +{ >> + bool cmd; >> + >> + if (rd_no_arg(rd) || !sys_valid_privileged_qkey_cmd(rd_argv(rd))) { >> + pr_err("valid options are: { on | off }\n"); >> + return -EINVAL; >> + } > This could use parse_on_off(). More importantly I looked a bit more into it, and I prefer not to use it, it would also lead to additional error prints that are not consistent with what we previously had for this API, so I prefer to keep it as is , so that the error messages for all arguments of this command be identical. > >> + >> + cmd = (strcmp(rd_argv(rd), "on") == 0) ? true : false; >> + >> + return sys_set_privileged_qkey_cmd(rd, cmd); >> +} >> + >> static int sys_set_help(struct rd *rd) >> { >> pr_out("Usage: %s system set [PARAM] value\n", rd->filename); >> pr_out(" system set netns { shared | exclusive }\n"); >> + pr_out(" system set privileged-qkey { on | off }\n"); >> return 0; >> } >> >> @@ -124,6 +183,7 @@ static int sys_set(struct rd *rd) >> { NULL, sys_set_help }, >> { "help", sys_set_help }, >> { "netns", sys_set_netns_args}, >> + { "privileged-qkey", sys_set_privileged_qkey_args}, >> { 0 } >> }; > The rest of the code looks sane to me, but I'm not familiar with the > feature.
On 10/22/23 1:41 AM, Patrisious Haddad wrote: > > On 10/19/2023 1:38 PM, Petr Machata wrote: >> Patrisious Haddad <phaddad@nvidia.com> writes: >> >>> @@ -40,6 +45,22 @@ static int sys_show_parse_cb(const struct nlmsghdr >>> *nlh, void *data) >>> mode_str); >>> } >>> + if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) { >>> + const char *pqkey_str; >>> + uint8_t pqkey_mode; >>> + >>> + pqkey_mode = >>> + >>> mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]); >>> + >>> + if (pqkey_mode < ARRAY_SIZE(privileged_qkey_str)) >>> + pqkey_str = privileged_qkey_str[pqkey_mode]; >>> + else >>> + pqkey_str = "unknown"; >>> + >>> + print_color_string(PRINT_ANY, COLOR_NONE, "privileged-qkey", >>> + "privileged-qkey %s ", pqkey_str); >>> + } >>> + >> Elsewhere in the file, you just use print_color_on_off(), why not here? > > The print_color_on_off was used for copy-on-fork which as you see has no > set function, > > I was simply trying to be consistent with this file convention & style, > whereas print_color_string was used for the other configurable value > ("netns"), I can obviously change that if you all see it as necessary. > >> >>> if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]) >>> cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]); >>> @@ -111,10 +155,25 @@ static int sys_set_netns_args(struct rd *rd) >>> return sys_set_netns_cmd(rd, cmd); >>> } >>> +static int sys_set_privileged_qkey_args(struct rd *rd) >>> +{ >>> + bool cmd; >>> + >>> + if (rd_no_arg(rd) || !sys_valid_privileged_qkey_cmd(rd_argv(rd))) { >>> + pr_err("valid options are: { on | off }\n"); >>> + return -EINVAL; >>> + } >> This could use parse_on_off(). > You are absolutely correct, but just as well was trying to maintain same > code style as the previous configurable value we have here, but I think > using parse_on_off here can save us some code. >> >>> + >>> + cmd = (strcmp(rd_argv(rd), "on") == 0) ? true : false; >>> + >>> + return sys_set_privileged_qkey_cmd(rd, cmd); >>> +} >>> + >>> static int sys_set_help(struct rd *rd) >>> { >>> pr_out("Usage: %s system set [PARAM] value\n", rd->filename); >>> pr_out(" system set netns { shared | exclusive }\n"); >>> + pr_out(" system set privileged-qkey { on | off }\n"); >>> return 0; >>> } >>> @@ -124,6 +183,7 @@ static int sys_set(struct rd *rd) >>> { NULL, sys_set_help }, >>> { "help", sys_set_help }, >>> { "netns", sys_set_netns_args}, >>> + { "privileged-qkey", sys_set_privileged_qkey_args}, >>> { 0 } >>> }; >> The rest of the code looks sane to me, but I'm not familiar with the >> feature. > If no one else has any comments soon, and these two comments are > actually considered critical I can re-send my patches with those issues > fixed. tools packaged with iproute2 should use common code where possible.
On 10/22/2023 7:48 PM, David Ahern wrote: > External email: Use caution opening links or attachments > > > On 10/22/23 1:41 AM, Patrisious Haddad wrote: >> On 10/19/2023 1:38 PM, Petr Machata wrote: >>> Patrisious Haddad <phaddad@nvidia.com> writes: >>> >>>> @@ -40,6 +45,22 @@ static int sys_show_parse_cb(const struct nlmsghdr >>>> *nlh, void *data) >>>> mode_str); >>>> } >>>> + if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) { >>>> + const char *pqkey_str; >>>> + uint8_t pqkey_mode; >>>> + >>>> + pqkey_mode = >>>> + >>>> mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]); >>>> + >>>> + if (pqkey_mode < ARRAY_SIZE(privileged_qkey_str)) >>>> + pqkey_str = privileged_qkey_str[pqkey_mode]; >>>> + else >>>> + pqkey_str = "unknown"; >>>> + >>>> + print_color_string(PRINT_ANY, COLOR_NONE, "privileged-qkey", >>>> + "privileged-qkey %s ", pqkey_str); >>>> + } >>>> + >>> Elsewhere in the file, you just use print_color_on_off(), why not here? >> The print_color_on_off was used for copy-on-fork which as you see has no >> set function, >> >> I was simply trying to be consistent with this file convention & style, >> whereas print_color_string was used for the other configurable value >> ("netns"), I can obviously change that if you all see it as necessary. >> >>>> if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]) >>>> cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]); >>>> @@ -111,10 +155,25 @@ static int sys_set_netns_args(struct rd *rd) >>>> return sys_set_netns_cmd(rd, cmd); >>>> } >>>> +static int sys_set_privileged_qkey_args(struct rd *rd) >>>> +{ >>>> + bool cmd; >>>> + >>>> + if (rd_no_arg(rd) || !sys_valid_privileged_qkey_cmd(rd_argv(rd))) { >>>> + pr_err("valid options are: { on | off }\n"); >>>> + return -EINVAL; >>>> + } >>> This could use parse_on_off(). >> You are absolutely correct, but just as well was trying to maintain same >> code style as the previous configurable value we have here, but I think >> using parse_on_off here can save us some code. >>>> + >>>> + cmd = (strcmp(rd_argv(rd), "on") == 0) ? true : false; >>>> + >>>> + return sys_set_privileged_qkey_cmd(rd, cmd); >>>> +} >>>> + >>>> static int sys_set_help(struct rd *rd) >>>> { >>>> pr_out("Usage: %s system set [PARAM] value\n", rd->filename); >>>> pr_out(" system set netns { shared | exclusive }\n"); >>>> + pr_out(" system set privileged-qkey { on | off }\n"); >>>> return 0; >>>> } >>>> @@ -124,6 +183,7 @@ static int sys_set(struct rd *rd) >>>> { NULL, sys_set_help }, >>>> { "help", sys_set_help }, >>>> { "netns", sys_set_netns_args}, >>>> + { "privileged-qkey", sys_set_privileged_qkey_args}, >>>> { 0 } >>>> }; >>> The rest of the code looks sane to me, but I'm not familiar with the >>> feature. >> If no one else has any comments soon, and these two comments are >> actually considered critical I can re-send my patches with those issues >> fixed. > tools packaged with iproute2 should use common code where possible. Okay, good point, fixed both comments and sent a V2 , it actually resulted in a much cleaner code. - Thanks.
diff --git a/rdma/sys.c b/rdma/sys.c index fd785b25..32ca3444 100644 --- a/rdma/sys.c +++ b/rdma/sys.c @@ -17,6 +17,11 @@ static const char *netns_modes_str[] = { "shared", }; +static const char *privileged_qkey_str[] = { + "off", + "on", +}; + static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data) { struct nlattr *tb[RDMA_NLDEV_ATTR_MAX] = {}; @@ -40,6 +45,22 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data) mode_str); } + if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) { + const char *pqkey_str; + uint8_t pqkey_mode; + + pqkey_mode = + mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]); + + if (pqkey_mode < ARRAY_SIZE(privileged_qkey_str)) + pqkey_str = privileged_qkey_str[pqkey_mode]; + else + pqkey_str = "unknown"; + + print_color_string(PRINT_ANY, COLOR_NONE, "privileged-qkey", + "privileged-qkey %s ", pqkey_str); + } + if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]) cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]); @@ -67,8 +88,9 @@ static int sys_show_no_args(struct rd *rd) static int sys_show(struct rd *rd) { const struct rd_cmd cmds[] = { - { NULL, sys_show_no_args}, - { "netns", sys_show_no_args}, + { NULL, sys_show_no_args}, + { "netns", sys_show_no_args}, + { "privileged-qkey", sys_show_no_args}, { 0 } }; @@ -86,6 +108,17 @@ static int sys_set_netns_cmd(struct rd *rd, bool enable) return rd_sendrecv_msg(rd, seq); } +static int sys_set_privileged_qkey_cmd(struct rd *rd, bool enable) +{ + uint32_t seq; + + rd_prepare_msg(rd, RDMA_NLDEV_CMD_SYS_SET, + &seq, (NLM_F_REQUEST | NLM_F_ACK)); + mnl_attr_put_u8(rd->nlh, RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE, enable); + + return rd_sendrecv_msg(rd, seq); +} + static bool sys_valid_netns_cmd(const char *cmd) { int i; @@ -97,6 +130,17 @@ static bool sys_valid_netns_cmd(const char *cmd) return false; } +static bool sys_valid_privileged_qkey_cmd(const char *cmd) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(privileged_qkey_str); i++) { + if (!strcmp(cmd, privileged_qkey_str[i])) + return true; + } + return false; +} + static int sys_set_netns_args(struct rd *rd) { bool cmd; @@ -111,10 +155,25 @@ static int sys_set_netns_args(struct rd *rd) return sys_set_netns_cmd(rd, cmd); } +static int sys_set_privileged_qkey_args(struct rd *rd) +{ + bool cmd; + + if (rd_no_arg(rd) || !sys_valid_privileged_qkey_cmd(rd_argv(rd))) { + pr_err("valid options are: { on | off }\n"); + return -EINVAL; + } + + cmd = (strcmp(rd_argv(rd), "on") == 0) ? true : false; + + return sys_set_privileged_qkey_cmd(rd, cmd); +} + static int sys_set_help(struct rd *rd) { pr_out("Usage: %s system set [PARAM] value\n", rd->filename); pr_out(" system set netns { shared | exclusive }\n"); + pr_out(" system set privileged-qkey { on | off }\n"); return 0; } @@ -124,6 +183,7 @@ static int sys_set(struct rd *rd) { NULL, sys_set_help }, { "help", sys_set_help }, { "netns", sys_set_netns_args}, + { "privileged-qkey", sys_set_privileged_qkey_args}, { 0 } }; diff --git a/rdma/utils.c b/rdma/utils.c index 8a091c05..09985069 100644 --- a/rdma/utils.c +++ b/rdma/utils.c @@ -473,6 +473,7 @@ static const enum mnl_attr_data_type nldev_policy[RDMA_NLDEV_ATTR_MAX] = { [RDMA_NLDEV_ATTR_STAT_AUTO_MODE_MASK] = MNL_TYPE_U32, [RDMA_NLDEV_ATTR_DEV_DIM] = MNL_TYPE_U8, [RDMA_NLDEV_ATTR_RES_RAW] = MNL_TYPE_BINARY, + [RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE] = MNL_TYPE_U8, }; static int rd_attr_check(const struct nlattr *attr, int *typep)
Enrich rdmatool with an option to enable or disable privileged QKEY. When enabled, non-privileged users will be allowed to specify a controlled QKEY. By default this parameter is disabled in order to comply with IB spec. According to the IB specification rel-1.6, section 3.5.3: "QKEYs with the most significant bit set are considered controlled QKEYs, and a HCA does not allow a consumer to arbitrarily specify a controlled QKEY." This allows old applications which existed before the kernel commit: 0cadb4db79e1 ("RDMA/uverbs: Restrict usage of privileged QKEYs") they can use privileged QKEYs without being a privileged user to now be able to work again without being privileged granted they turn on this parameter. rdma tool command examples and output. $ rdma system show netns shared privileged-qkey off copy-on-fork on $ rdma system set privileged-qkey on $ rdma system show netns shared privileged-qkey on copy-on-fork on Signed-off-by: Patrisious Haddad <phaddad@nvidia.com> Reviewed-by: Michael Guralnik <michaelgur@nvidia.com> --- rdma/sys.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++-- rdma/utils.c | 1 + 2 files changed, 63 insertions(+), 2 deletions(-)