* [PATCH iproute2-next 0/3] Add support to set privileged qkey parameter @ 2023-10-19 8:21 Patrisious Haddad 2023-10-19 8:21 ` [PATCH iproute2-next 1/3] rdma: update uapi headers Patrisious Haddad ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Patrisious Haddad @ 2023-10-19 8:21 UTC (permalink / raw) To: jgg, leon, dsahern, stephen Cc: Patrisious Haddad, netdev, linux-rdma, linuxarm, linux-kernel, huangjunxian6, michaelgur This patchset adds support to enable or disable privileged QKEY. When enabled, non-privileged users will be allowed to specify a controlled QKEY. The corresponding kernel commit is 36ce80759f8c ("RDMA/core: Add support to set privileged qkey parameter") All the information regarding the added parameter and its usage are included in the commits below and the edited man page. Patrisious Haddad (3): rdma: update uapi headers rdma: Add an option to set privileged QKEY parameter rdma: Adjust man page for rdma system set privileged_qkey command man/man8/rdma-system.8 | 32 +++++++++++--- rdma/include/uapi/rdma/rdma_netlink.h | 6 +++ rdma/sys.c | 64 ++++++++++++++++++++++++++- rdma/utils.c | 1 + 4 files changed, 96 insertions(+), 7 deletions(-) -- 2.18.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH iproute2-next 1/3] rdma: update uapi headers 2023-10-19 8:21 [PATCH iproute2-next 0/3] Add support to set privileged qkey parameter Patrisious Haddad @ 2023-10-19 8:21 ` Patrisious Haddad 2023-10-19 8:21 ` [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter Patrisious Haddad 2023-10-19 8:21 ` [PATCH iproute2-next 3/3] rdma: Adjust man page for rdma system set privileged_qkey command Patrisious Haddad 2 siblings, 0 replies; 10+ messages in thread From: Patrisious Haddad @ 2023-10-19 8:21 UTC (permalink / raw) To: jgg, leon, dsahern, stephen Cc: Patrisious Haddad, netdev, linux-rdma, linuxarm, linux-kernel, huangjunxian6, michaelgur Update rdma_netlink.h file upto kernel commit 36ce80759f8c ("RDMA/core: Add support to set privileged qkey parameter") Signed-off-by: Patrisious Haddad <phaddad@nvidia.com> Reviewed-by: Michael Guralnik <michaelgur@nvidia.com> --- rdma/include/uapi/rdma/rdma_netlink.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/rdma/include/uapi/rdma/rdma_netlink.h b/rdma/include/uapi/rdma/rdma_netlink.h index 92c528a0..3a506efa 100644 --- a/rdma/include/uapi/rdma/rdma_netlink.h +++ b/rdma/include/uapi/rdma/rdma_netlink.h @@ -554,6 +554,12 @@ enum rdma_nldev_attr { RDMA_NLDEV_ATTR_STAT_HWCOUNTER_INDEX, /* u32 */ RDMA_NLDEV_ATTR_STAT_HWCOUNTER_DYNAMIC, /* u8 */ + /* + * To enable or disable using privileged_qkey without being + * a privileged user. + */ + RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE, /* u8 */ + /* * Always the end */ -- 2.18.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter 2023-10-19 8:21 [PATCH iproute2-next 0/3] Add support to set privileged qkey parameter Patrisious Haddad 2023-10-19 8:21 ` [PATCH iproute2-next 1/3] rdma: update uapi headers Patrisious Haddad @ 2023-10-19 8:21 ` Patrisious Haddad 2023-10-19 10:38 ` Petr Machata 2023-10-19 8:21 ` [PATCH iproute2-next 3/3] rdma: Adjust man page for rdma system set privileged_qkey command Patrisious Haddad 2 siblings, 1 reply; 10+ messages in thread From: Patrisious Haddad @ 2023-10-19 8:21 UTC (permalink / raw) To: jgg, leon, dsahern, stephen Cc: Patrisious Haddad, netdev, linux-rdma, linuxarm, linux-kernel, huangjunxian6, michaelgur 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(-) 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) -- 2.18.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter 2023-10-19 8:21 ` [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter Patrisious Haddad @ 2023-10-19 10:38 ` Petr Machata 2023-10-19 15:05 ` David Ahern ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Petr Machata @ 2023-10-19 10:38 UTC (permalink / raw) To: Patrisious Haddad Cc: jgg, leon, dsahern, stephen, netdev, linux-rdma, linuxarm, linux-kernel, huangjunxian6, michaelgur 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter 2023-10-19 10:38 ` Petr Machata @ 2023-10-19 15:05 ` David Ahern 2023-10-22 7:41 ` Patrisious Haddad 2023-10-22 9:22 ` Patrisious Haddad 2 siblings, 0 replies; 10+ messages in thread From: David Ahern @ 2023-10-19 15:05 UTC (permalink / raw) To: Petr Machata, Patrisious Haddad Cc: jgg, leon, stephen, netdev, linux-rdma, linuxarm, linux-kernel, huangjunxian6, michaelgur 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter 2023-10-19 10:38 ` Petr Machata 2023-10-19 15:05 ` David Ahern @ 2023-10-22 7:41 ` Patrisious Haddad 2023-10-22 16:48 ` David Ahern 2023-10-22 9:22 ` Patrisious Haddad 2 siblings, 1 reply; 10+ messages in thread From: Patrisious Haddad @ 2023-10-22 7:41 UTC (permalink / raw) To: Petr Machata Cc: jgg, leon, dsahern, stephen, netdev, linux-rdma, linuxarm, linux-kernel, huangjunxian6, michaelgur 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter 2023-10-22 7:41 ` Patrisious Haddad @ 2023-10-22 16:48 ` David Ahern 2023-10-23 11:24 ` Patrisious Haddad 0 siblings, 1 reply; 10+ messages in thread From: David Ahern @ 2023-10-22 16:48 UTC (permalink / raw) To: Patrisious Haddad, Petr Machata Cc: jgg, leon, stephen, netdev, linux-rdma, linuxarm, linux-kernel, huangjunxian6, michaelgur 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter 2023-10-22 16:48 ` David Ahern @ 2023-10-23 11:24 ` Patrisious Haddad 0 siblings, 0 replies; 10+ messages in thread From: Patrisious Haddad @ 2023-10-23 11:24 UTC (permalink / raw) To: David Ahern, Petr Machata Cc: jgg, leon, stephen, netdev, linux-rdma, linuxarm, linux-kernel, huangjunxian6, michaelgur 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter 2023-10-19 10:38 ` Petr Machata 2023-10-19 15:05 ` David Ahern 2023-10-22 7:41 ` Patrisious Haddad @ 2023-10-22 9:22 ` Patrisious Haddad 2 siblings, 0 replies; 10+ messages in thread From: Patrisious Haddad @ 2023-10-22 9:22 UTC (permalink / raw) To: Petr Machata Cc: jgg, leon, dsahern, stephen, netdev, linux-rdma, linuxarm, linux-kernel, huangjunxian6, michaelgur 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH iproute2-next 3/3] rdma: Adjust man page for rdma system set privileged_qkey command 2023-10-19 8:21 [PATCH iproute2-next 0/3] Add support to set privileged qkey parameter Patrisious Haddad 2023-10-19 8:21 ` [PATCH iproute2-next 1/3] rdma: update uapi headers Patrisious Haddad 2023-10-19 8:21 ` [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter Patrisious Haddad @ 2023-10-19 8:21 ` Patrisious Haddad 2 siblings, 0 replies; 10+ messages in thread From: Patrisious Haddad @ 2023-10-19 8:21 UTC (permalink / raw) To: jgg, leon, dsahern, stephen Cc: Patrisious Haddad, netdev, linux-rdma, linuxarm, linux-kernel, huangjunxian6, michaelgur Signed-off-by: Patrisious Haddad <phaddad@nvidia.com> Reviewed-by: Michael Guralnik <michaelgur@nvidia.com> --- man/man8/rdma-system.8 | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/man/man8/rdma-system.8 b/man/man8/rdma-system.8 index ab1d89fd..a2914eb8 100644 --- a/man/man8/rdma-system.8 +++ b/man/man8/rdma-system.8 @@ -23,16 +23,16 @@ rdma-system \- RDMA subsystem configuration .ti -8 .B rdma system set -.BR netns -.BR NEWMODE +.BR netns/privileged_qkey +.BR NEWMODE/NEWSTATE .ti -8 .B rdma system help .SH "DESCRIPTION" -.SS rdma system set - set RDMA subsystem network namespace mode +.SS rdma system set - set RDMA subsystem network namespace mode or privileged qkey mode -.SS rdma system show - display RDMA subsystem network namespace mode +.SS rdma system show - display RDMA subsystem network namespace mode and privileged qkey state .PP .I "NEWMODE" @@ -49,12 +49,21 @@ network namespaces is not needed, shared mode can be used. It is preferred to not change the subsystem mode when there is active RDMA traffic running, even though it is supported. +.PP +.I "NEWSTATE" +- specifies the new state of the privileged_qkey parameter. Either enabled or disabled. +Whereas this decides whether a non-privileged user is allowed to specify a controlled +QKEY or not, since such QKEYS are considered privileged. + +When this parameter is enabled, non-privileged users will be allowed to +specify a controlled QKEY. .SH "EXAMPLES" .PP rdma system show .RS 4 -Shows the state of RDMA subsystem network namespace mode on the system. +Shows the state of RDMA subsystem network namespace mode on the system and +the state of privileged qkey parameter. .RE .PP rdma system set netns exclusive @@ -69,6 +78,19 @@ Sets the RDMA subsystem in network namespace shared mode. In this mode RDMA devi are shared among network namespaces. .RE .PP +.PP +rdma system set privileged_qkey enabled +.RS 4 +Sets the privileged_qkey parameter to enabled. In this state non-privileged user +is allowed to specify a controlled QKEY. +.RE +.PP +rdma system set privileged_qkey disabled +.RS 4 +Sets the privileged_qkey parameter to disabled. In this state non-privileged user +is *not* allowed to specify a controlled QKEY. +.RE +.PP .SH SEE ALSO .BR rdma (8), -- 2.18.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-10-23 11:25 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-10-19 8:21 [PATCH iproute2-next 0/3] Add support to set privileged qkey parameter Patrisious Haddad 2023-10-19 8:21 ` [PATCH iproute2-next 1/3] rdma: update uapi headers Patrisious Haddad 2023-10-19 8:21 ` [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter Patrisious Haddad 2023-10-19 10:38 ` Petr Machata 2023-10-19 15:05 ` David Ahern 2023-10-22 7:41 ` Patrisious Haddad 2023-10-22 16:48 ` David Ahern 2023-10-23 11:24 ` Patrisious Haddad 2023-10-22 9:22 ` Patrisious Haddad 2023-10-19 8:21 ` [PATCH iproute2-next 3/3] rdma: Adjust man page for rdma system set privileged_qkey command Patrisious Haddad
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).