| Message ID | 20231023112217.3439-3-phaddad@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | Add support to set privileged qkey parameter | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Patrisious Haddad <phaddad@nvidia.com> writes: > 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> Again, I'm not familiar with the subject matter, but mechanically this looks OK to me. Reviewed-by: Petr Machata <petrm@nvidia.com>
On 10/23/23 5:22 AM, Patrisious Haddad wrote: > diff --git a/rdma/sys.c b/rdma/sys.c > index fd785b25..db34cb41 100644 > --- a/rdma/sys.c > +++ b/rdma/sys.c > @@ -40,6 +40,17 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data) > mode_str); > } > > + if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) { > + uint8_t pqkey_mode; > + > + pqkey_mode = > + mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]); just make it mode so it fits on one line. 40 characters for an attribute name .... I will never understand this fascination with writing a sentence for an attribute name. > + > + print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey", > + "privileged-qkey %s ", pqkey_mode); > + > + } > + > if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]) > cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]); > keep Petr's reviewed-by tag on the respin.
On 10/24/2023 8:02 PM, David Ahern wrote: > External email: Use caution opening links or attachments > > > On 10/23/23 5:22 AM, Patrisious Haddad wrote: >> diff --git a/rdma/sys.c b/rdma/sys.c >> index fd785b25..db34cb41 100644 >> --- a/rdma/sys.c >> +++ b/rdma/sys.c >> @@ -40,6 +40,17 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data) >> mode_str); >> } >> >> + if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) { >> + uint8_t pqkey_mode; >> + >> + pqkey_mode = >> + mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]); > just make it mode so it fits on one line. will do. > > 40 characters for an attribute name .... I will never understand this > fascination with writing a sentence for an attribute name. me neither, just following the naming convention in rdma/include/uapi/rdma/rdma_netlink.h sadly ... > >> + >> + print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey", >> + "privileged-qkey %s ", pqkey_mode); >> + >> + } >> + >> if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]) >> cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]); >> > keep Petr's reviewed-by tag on the respin. will do, just making sure I understand "the respin" , you mean when I re-send it ?
David Ahern <dsahern@gmail.com> writes: > On 10/23/23 5:22 AM, Patrisious Haddad wrote: >> diff --git a/rdma/sys.c b/rdma/sys.c >> index fd785b25..db34cb41 100644 >> --- a/rdma/sys.c >> +++ b/rdma/sys.c >> @@ -40,6 +40,17 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data) >> mode_str); >> } >> >> + if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) { >> + uint8_t pqkey_mode; >> + >> + pqkey_mode = >> + mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]); > > just make it mode so it fits on one line. > > 40 characters for an attribute name .... I will never understand this > fascination with writing a sentence for an attribute name. Yeah, I noticed this after the review, and figured oh whatever. But this would be better: int at = RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE; uint8_t pqkey_mode; pqkey_mode = mnl_attr_get_u8(tb[at]); print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey", "privileged-qkey %s ", pqkey_mode); >> + >> + print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey", >> + "privileged-qkey %s ", pqkey_mode); >> + >> + } >> + >> if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]) >> cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]); >> > > keep Petr's reviewed-by tag on the respin.
Patrisious Haddad <phaddad@nvidia.com> writes: > On 10/24/2023 8:02 PM, David Ahern wrote: > >>> + >>> + print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey", >>> + "privileged-qkey %s ", pqkey_mode); >>> + >>> + } >>> + >>> if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]) >>> cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]); >>> >> keep Petr's reviewed-by tag on the respin. > > will do, just making sure I understand "the respin" , you mean when I > re-send it ? Yes, to send a new version with fixes.
diff --git a/rdma/sys.c b/rdma/sys.c index fd785b25..db34cb41 100644 --- a/rdma/sys.c +++ b/rdma/sys.c @@ -40,6 +40,17 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data) mode_str); } + if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) { + uint8_t pqkey_mode; + + pqkey_mode = + mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]); + + print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey", + "privileged-qkey %s ", pqkey_mode); + + } + if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]) cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]); @@ -67,8 +78,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 +98,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; @@ -111,10 +134,28 @@ 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; + int ret; + + if (rd_no_arg(rd)) { + pr_err("valid options are: { on | off }\n"); + return -EINVAL; + } + + cmd = parse_on_off("privileged-qkey", rd_argv(rd), &ret); + if (ret) + return -EINVAL; + + 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 +165,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 | 46 ++++++++++++++++++++++++++++++++++++++++++++-- rdma/utils.c | 1 + 2 files changed, 45 insertions(+), 2 deletions(-)