| Message ID | 20231123181546.521488-9-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: introduce notifications filtering | expand |
On 11/23/23 19:15, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > Currently the user listening on a socket for devlink notifications > gets always all messages for all existing instances, even if he is > interested only in one of those. That may cause unnecessary overhead > on setups with thousands of instances present. > > User is currently able to narrow down the devlink objects replies > to dump commands by specifying select attributes. > > Allow similar approach for notifications. Introduce a new devlink > NOTIFY_FILTER_SET which the user passes the select attributes. Store > these per-socket and use them for filtering messages > during multicast send. > > Signed-off-by: Jiri Pirko <jiri@nvidia.com> > --- > v3->v4: > - rebased on top of genl_sk_priv_*() introduction > --- > Documentation/netlink/specs/devlink.yaml | 10 ++++ > include/uapi/linux/devlink.h | 2 + > net/devlink/devl_internal.h | 34 ++++++++++- > net/devlink/netlink.c | 73 ++++++++++++++++++++++++ > net/devlink/netlink_gen.c | 15 ++++- > net/devlink/netlink_gen.h | 4 +- > tools/net/ynl/generated/devlink-user.c | 31 ++++++++++ > tools/net/ynl/generated/devlink-user.h | 47 +++++++++++++++ > 8 files changed, 212 insertions(+), 4 deletions(-) > > diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml > index 43067e1f63aa..6bad1d3454b7 100644 > --- a/Documentation/netlink/specs/devlink.yaml > +++ b/Documentation/netlink/specs/devlink.yaml > @@ -2055,3 +2055,13 @@ operations: > - bus-name > - dev-name > - selftests > + > + - > + name: notify-filter-set > + doc: Set notification messages socket filter. > + attribute-set: devlink > + do: > + request: > + attributes: > + - bus-name > + - dev-name > diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h > index b3c8383d342d..130cae0d3e20 100644 > --- a/include/uapi/linux/devlink.h > +++ b/include/uapi/linux/devlink.h > @@ -139,6 +139,8 @@ enum devlink_command { > DEVLINK_CMD_SELFTESTS_GET, /* can dump */ > DEVLINK_CMD_SELFTESTS_RUN, > > + DEVLINK_CMD_NOTIFY_FILTER_SET, > + > /* add new commands above here */ > __DEVLINK_CMD_MAX, > DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1 > diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h > index 84dc9628d3f2..82e0fb3bbebf 100644 > --- a/net/devlink/devl_internal.h > +++ b/net/devlink/devl_internal.h > @@ -191,11 +191,41 @@ static inline bool devlink_nl_notify_need(struct devlink *devlink) > DEVLINK_MCGRP_CONFIG); > } > > +struct devlink_obj_desc { > + struct rcu_head rcu; > + const char *bus_name; > + const char *dev_name; > + long data[]; could you please remove that data pointer?, you are not using desc as flex pointer as of now > +}; > + > +static inline void devlink_nl_obj_desc_init(struct devlink_obj_desc *desc, given next patch of the series with port index, you could rename this function to devlink_nl_obj_desc_names_set(), and move 0-init outside. > + struct devlink *devlink) > +{ > + memset(desc, 0, sizeof(*desc)); > + desc->bus_name = devlink->dev->bus->name; > + desc->dev_name = dev_name(devlink->dev); > +} > + > +int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data); > + > +static inline void devlink_nl_notify_send_desc(struct devlink *devlink, > + struct sk_buff *msg, > + struct devlink_obj_desc *desc) > +{ > + genlmsg_multicast_netns_filtered(&devlink_nl_family, > + devlink_net(devlink), > + msg, 0, DEVLINK_MCGRP_CONFIG, > + GFP_KERNEL, > + devlink_nl_notify_filter, desc); > +} > + > static inline void devlink_nl_notify_send(struct devlink *devlink, > struct sk_buff *msg) > { > - genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink), > - msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL); > + struct devlink_obj_desc desc; `= {};` would wipe out the need for memset(). > + > + devlink_nl_obj_desc_init(&desc, devlink); > + devlink_nl_notify_send_desc(devlink, msg, &desc); > } > > /* Notify */ [snip]
Mon, Nov 27, 2023 at 01:30:04PM CET, przemyslaw.kitszel@intel.com wrote: >On 11/23/23 19:15, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@nvidia.com> >> >> Currently the user listening on a socket for devlink notifications >> gets always all messages for all existing instances, even if he is >> interested only in one of those. That may cause unnecessary overhead >> on setups with thousands of instances present. >> >> User is currently able to narrow down the devlink objects replies >> to dump commands by specifying select attributes. >> >> Allow similar approach for notifications. Introduce a new devlink >> NOTIFY_FILTER_SET which the user passes the select attributes. Store >> these per-socket and use them for filtering messages >> during multicast send. >> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com> >> --- >> v3->v4: >> - rebased on top of genl_sk_priv_*() introduction >> --- >> Documentation/netlink/specs/devlink.yaml | 10 ++++ >> include/uapi/linux/devlink.h | 2 + >> net/devlink/devl_internal.h | 34 ++++++++++- >> net/devlink/netlink.c | 73 ++++++++++++++++++++++++ >> net/devlink/netlink_gen.c | 15 ++++- >> net/devlink/netlink_gen.h | 4 +- >> tools/net/ynl/generated/devlink-user.c | 31 ++++++++++ >> tools/net/ynl/generated/devlink-user.h | 47 +++++++++++++++ >> 8 files changed, 212 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml >> index 43067e1f63aa..6bad1d3454b7 100644 >> --- a/Documentation/netlink/specs/devlink.yaml >> +++ b/Documentation/netlink/specs/devlink.yaml >> @@ -2055,3 +2055,13 @@ operations: >> - bus-name >> - dev-name >> - selftests >> + >> + - >> + name: notify-filter-set >> + doc: Set notification messages socket filter. >> + attribute-set: devlink >> + do: >> + request: >> + attributes: >> + - bus-name >> + - dev-name >> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h >> index b3c8383d342d..130cae0d3e20 100644 >> --- a/include/uapi/linux/devlink.h >> +++ b/include/uapi/linux/devlink.h >> @@ -139,6 +139,8 @@ enum devlink_command { >> DEVLINK_CMD_SELFTESTS_GET, /* can dump */ >> DEVLINK_CMD_SELFTESTS_RUN, >> + DEVLINK_CMD_NOTIFY_FILTER_SET, >> + >> /* add new commands above here */ >> __DEVLINK_CMD_MAX, >> DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1 >> diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h >> index 84dc9628d3f2..82e0fb3bbebf 100644 >> --- a/net/devlink/devl_internal.h >> +++ b/net/devlink/devl_internal.h >> @@ -191,11 +191,41 @@ static inline bool devlink_nl_notify_need(struct devlink *devlink) >> DEVLINK_MCGRP_CONFIG); >> } >> +struct devlink_obj_desc { >> + struct rcu_head rcu; >> + const char *bus_name; >> + const char *dev_name; >> + long data[]; > >could you please remove that data pointer?, >you are not using desc as flex pointer as of now But I am. See devlink_nl_notify_filter_set_doit() > >> +}; >> + >> +static inline void devlink_nl_obj_desc_init(struct devlink_obj_desc *desc, > >given next patch of the series with port index, you could rename this >function to devlink_nl_obj_desc_names_set(), and move 0-init outside. I don't see why. This init will be called all the time, even if in future more attrs selection is going to be used. > >> + struct devlink *devlink) >> +{ >> + memset(desc, 0, sizeof(*desc)); >> + desc->bus_name = devlink->dev->bus->name; >> + desc->dev_name = dev_name(devlink->dev); >> +} >> + >> +int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data); >> + >> +static inline void devlink_nl_notify_send_desc(struct devlink *devlink, >> + struct sk_buff *msg, >> + struct devlink_obj_desc *desc) >> +{ >> + genlmsg_multicast_netns_filtered(&devlink_nl_family, >> + devlink_net(devlink), >> + msg, 0, DEVLINK_MCGRP_CONFIG, >> + GFP_KERNEL, >> + devlink_nl_notify_filter, desc); >> +} >> + >> static inline void devlink_nl_notify_send(struct devlink *devlink, >> struct sk_buff *msg) >> { >> - genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink), >> - msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL); >> + struct devlink_obj_desc desc; > >`= {};` would wipe out the need for memset(). True. If there is going to be a respin, I'll change this. > >> + >> + devlink_nl_obj_desc_init(&desc, devlink); >> + devlink_nl_notify_send_desc(devlink, msg, &desc); >> } >> /* Notify */ > >[snip]
Mon, Nov 27, 2023 at 01:51:03PM CET, jiri@resnulli.us wrote: >Mon, Nov 27, 2023 at 01:30:04PM CET, przemyslaw.kitszel@intel.com wrote: >>On 11/23/23 19:15, Jiri Pirko wrote: >>> From: Jiri Pirko <jiri@nvidia.com> [...] >>> static inline void devlink_nl_notify_send(struct devlink *devlink, >>> struct sk_buff *msg) >>> { >>> - genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink), >>> - msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL); >>> + struct devlink_obj_desc desc; >> >>`= {};` would wipe out the need for memset(). > >True. If there is going to be a respin, I'll change this. On a second thought, since the next patch adds couple more users of devlink_nl_obj_desc_init(), I prefer to leave the zeroing there. > > >> >>> + >>> + devlink_nl_obj_desc_init(&desc, devlink); >>> + devlink_nl_notify_send_desc(devlink, msg, &desc); >>> } >>> /* Notify */ >> >>[snip]
On 11/23/23 19:15, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > Currently the user listening on a socket for devlink notifications > gets always all messages for all existing instances, even if he is > interested only in one of those. That may cause unnecessary overhead > on setups with thousands of instances present. > > User is currently able to narrow down the devlink objects replies > to dump commands by specifying select attributes. > > Allow similar approach for notifications. Introduce a new devlink > NOTIFY_FILTER_SET which the user passes the select attributes. Store > these per-socket and use them for filtering messages > during multicast send. > > Signed-off-by: Jiri Pirko <jiri@nvidia.com> > --- > v3->v4: > - rebased on top of genl_sk_priv_*() introduction > --- > Documentation/netlink/specs/devlink.yaml | 10 ++++ > include/uapi/linux/devlink.h | 2 + > net/devlink/devl_internal.h | 34 ++++++++++- > net/devlink/netlink.c | 73 ++++++++++++++++++++++++ > net/devlink/netlink_gen.c | 15 ++++- > net/devlink/netlink_gen.h | 4 +- > tools/net/ynl/generated/devlink-user.c | 31 ++++++++++ > tools/net/ynl/generated/devlink-user.h | 47 +++++++++++++++ > 8 files changed, 212 insertions(+), 4 deletions(-) > > diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml > index 43067e1f63aa..6bad1d3454b7 100644 > --- a/Documentation/netlink/specs/devlink.yaml > +++ b/Documentation/netlink/specs/devlink.yaml > @@ -2055,3 +2055,13 @@ operations: > - bus-name > - dev-name > - selftests > + > + - > + name: notify-filter-set > + doc: Set notification messages socket filter. > + attribute-set: devlink > + do: > + request: > + attributes: > + - bus-name > + - dev-name > diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h > index b3c8383d342d..130cae0d3e20 100644 > --- a/include/uapi/linux/devlink.h > +++ b/include/uapi/linux/devlink.h > @@ -139,6 +139,8 @@ enum devlink_command { > DEVLINK_CMD_SELFTESTS_GET, /* can dump */ > DEVLINK_CMD_SELFTESTS_RUN, > > + DEVLINK_CMD_NOTIFY_FILTER_SET, > + > /* add new commands above here */ > __DEVLINK_CMD_MAX, > DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1 > diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h > index 84dc9628d3f2..82e0fb3bbebf 100644 > --- a/net/devlink/devl_internal.h > +++ b/net/devlink/devl_internal.h > @@ -191,11 +191,41 @@ static inline bool devlink_nl_notify_need(struct devlink *devlink) > DEVLINK_MCGRP_CONFIG); > } > > +struct devlink_obj_desc { > + struct rcu_head rcu; > + const char *bus_name; > + const char *dev_name; > + long data[]; > +}; > + > +static inline void devlink_nl_obj_desc_init(struct devlink_obj_desc *desc, > + struct devlink *devlink) > +{ > + memset(desc, 0, sizeof(*desc)); > + desc->bus_name = devlink->dev->bus->name; > + desc->dev_name = dev_name(devlink->dev); > +} > + > +int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data); > + > +static inline void devlink_nl_notify_send_desc(struct devlink *devlink, > + struct sk_buff *msg, > + struct devlink_obj_desc *desc) > +{ > + genlmsg_multicast_netns_filtered(&devlink_nl_family, > + devlink_net(devlink), > + msg, 0, DEVLINK_MCGRP_CONFIG, > + GFP_KERNEL, > + devlink_nl_notify_filter, desc); > +} > + > static inline void devlink_nl_notify_send(struct devlink *devlink, > struct sk_buff *msg) > { > - genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink), > - msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL); > + struct devlink_obj_desc desc; > + > + devlink_nl_obj_desc_init(&desc, devlink); > + devlink_nl_notify_send_desc(devlink, msg, &desc); > } > > /* Notify */ > diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c > index fa9afe3e6d9b..33a8e51dea68 100644 > --- a/net/devlink/netlink.c > +++ b/net/devlink/netlink.c > @@ -17,6 +17,79 @@ static const struct genl_multicast_group devlink_nl_mcgrps[] = { > [DEVLINK_MCGRP_CONFIG] = { .name = DEVLINK_GENL_MCGRP_CONFIG_NAME }, > }; > > +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb, > + struct genl_info *info) > +{ > + struct nlattr **attrs = info->attrs; > + struct devlink_obj_desc *flt; > + size_t data_offset = 0; > + size_t data_size = 0; > + char *pos; > + > + if (attrs[DEVLINK_ATTR_BUS_NAME]) > + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1; > + if (attrs[DEVLINK_ATTR_DEV_NAME]) > + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1; > + > + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL); instead of arithmetic here, you could use struct_size() > + if (!flt) > + return -ENOMEM; > + > + pos = (char *) flt->data; > + if (attrs[DEVLINK_ATTR_BUS_NAME]) { > + data_offset += nla_strscpy(pos, > + attrs[DEVLINK_ATTR_BUS_NAME], > + data_size) + 1; > + flt->bus_name = pos; > + pos += data_offset; > + } > + if (attrs[DEVLINK_ATTR_DEV_NAME]) { > + nla_strscpy(pos, attrs[DEVLINK_ATTR_DEV_NAME], > + data_size - data_offset); > + flt->dev_name = pos; > + } > + > + /* Don't attach empty filter. */ > + if (!flt->bus_name && !flt->dev_name) { > + kfree(flt); > + flt = NULL; > + } > + (Thanks for pointing out to this place in the other sub-thread) [here1] Assume that @flt is fine here. > + flt = genl_sk_priv_store(NETLINK_CB(skb).sk, &devlink_nl_family, flt); > + if (IS_ERR(flt)) > + return PTR_ERR(flt); and now you got an error from genl_sk_priv_store(), which means that you leak old flt as of [here1]. I am correct? (sorry it's kinda late :/) > + else if (flt) > + kfree_rcu(flt, rcu); > + > + return 0; > +} > + > +static bool devlink_obj_desc_match(const struct devlink_obj_desc *desc, > + const struct devlink_obj_desc *flt) > +{ > + if (desc->bus_name && flt->bus_name && > + strcmp(desc->bus_name, flt->bus_name)) > + return false; > + if (desc->dev_name && flt->dev_name && > + strcmp(desc->dev_name, flt->dev_name)) > + return false; > + return true; > +} > + > +int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data) > +{ > + struct devlink_obj_desc *desc = data; > + struct devlink_obj_desc *flt; > + int ret = 0; > + > + rcu_read_lock(); > + flt = genl_sk_priv_get(dsk, &devlink_nl_family); > + if (flt) > + ret = !devlink_obj_desc_match(desc, flt); > + rcu_read_unlock(); > + return ret; > +} > + > int devlink_nl_put_nested_handle(struct sk_buff *msg, struct net *net, > struct devlink *devlink, int attrtype) > { > diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c > index 95f9b4350ab7..1cb0e05305d2 100644 > --- a/net/devlink/netlink_gen.c > +++ b/net/devlink/netlink_gen.c > @@ -560,8 +560,14 @@ static const struct nla_policy devlink_selftests_run_nl_policy[DEVLINK_ATTR_SELF > [DEVLINK_ATTR_SELFTESTS] = NLA_POLICY_NESTED(devlink_dl_selftest_id_nl_policy), > }; > > +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */ > +static const struct nla_policy devlink_notify_filter_set_nl_policy[DEVLINK_ATTR_DEV_NAME + 1] = { > + [DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, }, > + [DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, }, > +}; > + > /* Ops table for devlink */ > -const struct genl_split_ops devlink_nl_ops[73] = { > +const struct genl_split_ops devlink_nl_ops[74] = { > { > .cmd = DEVLINK_CMD_GET, > .validate = GENL_DONT_VALIDATE_STRICT, > @@ -1233,4 +1239,11 @@ const struct genl_split_ops devlink_nl_ops[73] = { > .maxattr = DEVLINK_ATTR_SELFTESTS, > .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, > }, > + { > + .cmd = DEVLINK_CMD_NOTIFY_FILTER_SET, > + .doit = devlink_nl_notify_filter_set_doit, > + .policy = devlink_notify_filter_set_nl_policy, > + .maxattr = DEVLINK_ATTR_DEV_NAME, > + .flags = GENL_CMD_CAP_DO, > + }, > }; > diff --git a/net/devlink/netlink_gen.h b/net/devlink/netlink_gen.h > index 02f3c0bfae0e..8f2bd50ddf5e 100644 > --- a/net/devlink/netlink_gen.h > +++ b/net/devlink/netlink_gen.h > @@ -16,7 +16,7 @@ extern const struct nla_policy devlink_dl_port_function_nl_policy[DEVLINK_PORT_F > extern const struct nla_policy devlink_dl_selftest_id_nl_policy[DEVLINK_ATTR_SELFTEST_ID_FLASH + 1]; > > /* Ops table for devlink */ > -extern const struct genl_split_ops devlink_nl_ops[73]; > +extern const struct genl_split_ops devlink_nl_ops[74]; > > int devlink_nl_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, > struct genl_info *info); > @@ -142,5 +142,7 @@ int devlink_nl_selftests_get_doit(struct sk_buff *skb, struct genl_info *info); > int devlink_nl_selftests_get_dumpit(struct sk_buff *skb, > struct netlink_callback *cb); > int devlink_nl_selftests_run_doit(struct sk_buff *skb, struct genl_info *info); > +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb, > + struct genl_info *info); > > #endif /* _LINUX_DEVLINK_GEN_H */ > diff --git a/tools/net/ynl/generated/devlink-user.c b/tools/net/ynl/generated/devlink-user.c > index bc5065bd99b2..cd5f70eadf5b 100644 > --- a/tools/net/ynl/generated/devlink-user.c > +++ b/tools/net/ynl/generated/devlink-user.c > @@ -6830,6 +6830,37 @@ int devlink_selftests_run(struct ynl_sock *ys, > return 0; > } > > +/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */ > +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */ > +void > +devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req) > +{ > + free(req->bus_name); > + free(req->dev_name); > + free(req); > +} > + > +int devlink_notify_filter_set(struct ynl_sock *ys, > + struct devlink_notify_filter_set_req *req) > +{ > + struct nlmsghdr *nlh; > + int err; > + > + nlh = ynl_gemsg_start_req(ys, ys->family_id, DEVLINK_CMD_NOTIFY_FILTER_SET, 1); > + ys->req_policy = &devlink_nest; > + > + if (req->_present.bus_name_len) > + mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, req->bus_name); > + if (req->_present.dev_name_len) > + mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, req->dev_name); > + > + err = ynl_exec(ys, nlh, NULL); > + if (err < 0) > + return -1; > + > + return 0; > +} > + > const struct ynl_family ynl_devlink_family = { > .name = "devlink", > }; > diff --git a/tools/net/ynl/generated/devlink-user.h b/tools/net/ynl/generated/devlink-user.h > index 1db4edc36eaa..e5d79b824a67 100644 > --- a/tools/net/ynl/generated/devlink-user.h > +++ b/tools/net/ynl/generated/devlink-user.h > @@ -5252,4 +5252,51 @@ devlink_selftests_run_req_set_selftests_flash(struct devlink_selftests_run_req * > int devlink_selftests_run(struct ynl_sock *ys, > struct devlink_selftests_run_req *req); > > +/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */ > +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */ > +struct devlink_notify_filter_set_req { > + struct { > + __u32 bus_name_len; > + __u32 dev_name_len; > + } _present; > + > + char *bus_name; > + char *dev_name; > +}; > + > +static inline struct devlink_notify_filter_set_req * > +devlink_notify_filter_set_req_alloc(void) > +{ > + return calloc(1, sizeof(struct devlink_notify_filter_set_req)); > +} > +void > +devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req); > + > +static inline void > +devlink_notify_filter_set_req_set_bus_name(struct devlink_notify_filter_set_req *req, > + const char *bus_name) > +{ > + free(req->bus_name); > + req->_present.bus_name_len = strlen(bus_name); > + req->bus_name = malloc(req->_present.bus_name_len + 1); > + memcpy(req->bus_name, bus_name, req->_present.bus_name_len); > + req->bus_name[req->_present.bus_name_len] = 0; > +} > +static inline void > +devlink_notify_filter_set_req_set_dev_name(struct devlink_notify_filter_set_req *req, > + const char *dev_name) > +{ > + free(req->dev_name); > + req->_present.dev_name_len = strlen(dev_name); > + req->dev_name = malloc(req->_present.dev_name_len + 1); > + memcpy(req->dev_name, dev_name, req->_present.dev_name_len); > + req->dev_name[req->_present.dev_name_len] = 0; > +} > + > +/* > + * Set notification messages socket filter. > + */ > +int devlink_notify_filter_set(struct ynl_sock *ys, > + struct devlink_notify_filter_set_req *req); > + > #endif /* _LINUX_DEVLINK_GEN_H */
Mon, Nov 27, 2023 at 04:40:22PM CET, przemyslaw.kitszel@intel.com wrote: >On 11/23/23 19:15, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@nvidia.com> >> >> Currently the user listening on a socket for devlink notifications >> gets always all messages for all existing instances, even if he is >> interested only in one of those. That may cause unnecessary overhead >> on setups with thousands of instances present. >> >> User is currently able to narrow down the devlink objects replies >> to dump commands by specifying select attributes. >> >> Allow similar approach for notifications. Introduce a new devlink >> NOTIFY_FILTER_SET which the user passes the select attributes. Store >> these per-socket and use them for filtering messages >> during multicast send. >> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com> >> --- >> v3->v4: >> - rebased on top of genl_sk_priv_*() introduction >> --- >> Documentation/netlink/specs/devlink.yaml | 10 ++++ >> include/uapi/linux/devlink.h | 2 + >> net/devlink/devl_internal.h | 34 ++++++++++- >> net/devlink/netlink.c | 73 ++++++++++++++++++++++++ >> net/devlink/netlink_gen.c | 15 ++++- >> net/devlink/netlink_gen.h | 4 +- >> tools/net/ynl/generated/devlink-user.c | 31 ++++++++++ >> tools/net/ynl/generated/devlink-user.h | 47 +++++++++++++++ >> 8 files changed, 212 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml >> index 43067e1f63aa..6bad1d3454b7 100644 >> --- a/Documentation/netlink/specs/devlink.yaml >> +++ b/Documentation/netlink/specs/devlink.yaml >> @@ -2055,3 +2055,13 @@ operations: >> - bus-name >> - dev-name >> - selftests >> + >> + - >> + name: notify-filter-set >> + doc: Set notification messages socket filter. >> + attribute-set: devlink >> + do: >> + request: >> + attributes: >> + - bus-name >> + - dev-name >> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h >> index b3c8383d342d..130cae0d3e20 100644 >> --- a/include/uapi/linux/devlink.h >> +++ b/include/uapi/linux/devlink.h >> @@ -139,6 +139,8 @@ enum devlink_command { >> DEVLINK_CMD_SELFTESTS_GET, /* can dump */ >> DEVLINK_CMD_SELFTESTS_RUN, >> + DEVLINK_CMD_NOTIFY_FILTER_SET, >> + >> /* add new commands above here */ >> __DEVLINK_CMD_MAX, >> DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1 >> diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h >> index 84dc9628d3f2..82e0fb3bbebf 100644 >> --- a/net/devlink/devl_internal.h >> +++ b/net/devlink/devl_internal.h >> @@ -191,11 +191,41 @@ static inline bool devlink_nl_notify_need(struct devlink *devlink) >> DEVLINK_MCGRP_CONFIG); >> } >> +struct devlink_obj_desc { >> + struct rcu_head rcu; >> + const char *bus_name; >> + const char *dev_name; >> + long data[]; >> +}; >> + >> +static inline void devlink_nl_obj_desc_init(struct devlink_obj_desc *desc, >> + struct devlink *devlink) >> +{ >> + memset(desc, 0, sizeof(*desc)); >> + desc->bus_name = devlink->dev->bus->name; >> + desc->dev_name = dev_name(devlink->dev); >> +} >> + >> +int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data); >> + >> +static inline void devlink_nl_notify_send_desc(struct devlink *devlink, >> + struct sk_buff *msg, >> + struct devlink_obj_desc *desc) >> +{ >> + genlmsg_multicast_netns_filtered(&devlink_nl_family, >> + devlink_net(devlink), >> + msg, 0, DEVLINK_MCGRP_CONFIG, >> + GFP_KERNEL, >> + devlink_nl_notify_filter, desc); >> +} >> + >> static inline void devlink_nl_notify_send(struct devlink *devlink, >> struct sk_buff *msg) >> { >> - genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink), >> - msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL); >> + struct devlink_obj_desc desc; >> + >> + devlink_nl_obj_desc_init(&desc, devlink); >> + devlink_nl_notify_send_desc(devlink, msg, &desc); >> } >> /* Notify */ >> diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c >> index fa9afe3e6d9b..33a8e51dea68 100644 >> --- a/net/devlink/netlink.c >> +++ b/net/devlink/netlink.c >> @@ -17,6 +17,79 @@ static const struct genl_multicast_group devlink_nl_mcgrps[] = { >> [DEVLINK_MCGRP_CONFIG] = { .name = DEVLINK_GENL_MCGRP_CONFIG_NAME }, >> }; >> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb, >> + struct genl_info *info) >> +{ >> + struct nlattr **attrs = info->attrs; >> + struct devlink_obj_desc *flt; >> + size_t data_offset = 0; >> + size_t data_size = 0; >> + char *pos; >> + >> + if (attrs[DEVLINK_ATTR_BUS_NAME]) >> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1; >> + if (attrs[DEVLINK_ATTR_DEV_NAME]) >> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1; >> + >> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL); > >instead of arithmetic here, you could use struct_size() > >> + if (!flt) >> + return -ENOMEM; >> + >> + pos = (char *) flt->data; >> + if (attrs[DEVLINK_ATTR_BUS_NAME]) { >> + data_offset += nla_strscpy(pos, >> + attrs[DEVLINK_ATTR_BUS_NAME], >> + data_size) + 1; >> + flt->bus_name = pos; >> + pos += data_offset; >> + } >> + if (attrs[DEVLINK_ATTR_DEV_NAME]) { >> + nla_strscpy(pos, attrs[DEVLINK_ATTR_DEV_NAME], >> + data_size - data_offset); >> + flt->dev_name = pos; >> + } >> + >> + /* Don't attach empty filter. */ >> + if (!flt->bus_name && !flt->dev_name) { >> + kfree(flt); >> + flt = NULL; >> + } >> + > >(Thanks for pointing out to this place in the other sub-thread) > >[here1] Assume that @flt is fine here. > >> + flt = genl_sk_priv_store(NETLINK_CB(skb).sk, &devlink_nl_family, flt); >> + if (IS_ERR(flt)) >> + return PTR_ERR(flt); > >and now you got an error from genl_sk_priv_store(), >which means that you leak old flt as of [here1]. >I am correct? (sorry it's kinda late :/) You are correct. Thanks. Looks like I'll be rewriting this entirely anyway. > >> + else if (flt) >> + kfree_rcu(flt, rcu); >> + >> + return 0; >> +} >> + >> +static bool devlink_obj_desc_match(const struct devlink_obj_desc *desc, >> + const struct devlink_obj_desc *flt) >> +{ >> + if (desc->bus_name && flt->bus_name && >> + strcmp(desc->bus_name, flt->bus_name)) >> + return false; >> + if (desc->dev_name && flt->dev_name && >> + strcmp(desc->dev_name, flt->dev_name)) >> + return false; >> + return true; >> +} >> + >> +int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data) >> +{ >> + struct devlink_obj_desc *desc = data; >> + struct devlink_obj_desc *flt; >> + int ret = 0; >> + >> + rcu_read_lock(); >> + flt = genl_sk_priv_get(dsk, &devlink_nl_family); >> + if (flt) >> + ret = !devlink_obj_desc_match(desc, flt); >> + rcu_read_unlock(); >> + return ret; >> +} >> + >> int devlink_nl_put_nested_handle(struct sk_buff *msg, struct net *net, >> struct devlink *devlink, int attrtype) >> { >> diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c >> index 95f9b4350ab7..1cb0e05305d2 100644 >> --- a/net/devlink/netlink_gen.c >> +++ b/net/devlink/netlink_gen.c >> @@ -560,8 +560,14 @@ static const struct nla_policy devlink_selftests_run_nl_policy[DEVLINK_ATTR_SELF >> [DEVLINK_ATTR_SELFTESTS] = NLA_POLICY_NESTED(devlink_dl_selftest_id_nl_policy), >> }; >> +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */ >> +static const struct nla_policy devlink_notify_filter_set_nl_policy[DEVLINK_ATTR_DEV_NAME + 1] = { >> + [DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, }, >> + [DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, }, >> +}; >> + >> /* Ops table for devlink */ >> -const struct genl_split_ops devlink_nl_ops[73] = { >> +const struct genl_split_ops devlink_nl_ops[74] = { >> { >> .cmd = DEVLINK_CMD_GET, >> .validate = GENL_DONT_VALIDATE_STRICT, >> @@ -1233,4 +1239,11 @@ const struct genl_split_ops devlink_nl_ops[73] = { >> .maxattr = DEVLINK_ATTR_SELFTESTS, >> .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, >> }, >> + { >> + .cmd = DEVLINK_CMD_NOTIFY_FILTER_SET, >> + .doit = devlink_nl_notify_filter_set_doit, >> + .policy = devlink_notify_filter_set_nl_policy, >> + .maxattr = DEVLINK_ATTR_DEV_NAME, >> + .flags = GENL_CMD_CAP_DO, >> + }, >> }; >> diff --git a/net/devlink/netlink_gen.h b/net/devlink/netlink_gen.h >> index 02f3c0bfae0e..8f2bd50ddf5e 100644 >> --- a/net/devlink/netlink_gen.h >> +++ b/net/devlink/netlink_gen.h >> @@ -16,7 +16,7 @@ extern const struct nla_policy devlink_dl_port_function_nl_policy[DEVLINK_PORT_F >> extern const struct nla_policy devlink_dl_selftest_id_nl_policy[DEVLINK_ATTR_SELFTEST_ID_FLASH + 1]; >> /* Ops table for devlink */ >> -extern const struct genl_split_ops devlink_nl_ops[73]; >> +extern const struct genl_split_ops devlink_nl_ops[74]; >> int devlink_nl_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, >> struct genl_info *info); >> @@ -142,5 +142,7 @@ int devlink_nl_selftests_get_doit(struct sk_buff *skb, struct genl_info *info); >> int devlink_nl_selftests_get_dumpit(struct sk_buff *skb, >> struct netlink_callback *cb); >> int devlink_nl_selftests_run_doit(struct sk_buff *skb, struct genl_info *info); >> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb, >> + struct genl_info *info); >> #endif /* _LINUX_DEVLINK_GEN_H */ >> diff --git a/tools/net/ynl/generated/devlink-user.c b/tools/net/ynl/generated/devlink-user.c >> index bc5065bd99b2..cd5f70eadf5b 100644 >> --- a/tools/net/ynl/generated/devlink-user.c >> +++ b/tools/net/ynl/generated/devlink-user.c >> @@ -6830,6 +6830,37 @@ int devlink_selftests_run(struct ynl_sock *ys, >> return 0; >> } >> +/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */ >> +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */ >> +void >> +devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req) >> +{ >> + free(req->bus_name); >> + free(req->dev_name); >> + free(req); >> +} >> + >> +int devlink_notify_filter_set(struct ynl_sock *ys, >> + struct devlink_notify_filter_set_req *req) >> +{ >> + struct nlmsghdr *nlh; >> + int err; >> + >> + nlh = ynl_gemsg_start_req(ys, ys->family_id, DEVLINK_CMD_NOTIFY_FILTER_SET, 1); >> + ys->req_policy = &devlink_nest; >> + >> + if (req->_present.bus_name_len) >> + mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, req->bus_name); >> + if (req->_present.dev_name_len) >> + mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, req->dev_name); >> + >> + err = ynl_exec(ys, nlh, NULL); >> + if (err < 0) >> + return -1; >> + >> + return 0; >> +} >> + >> const struct ynl_family ynl_devlink_family = { >> .name = "devlink", >> }; >> diff --git a/tools/net/ynl/generated/devlink-user.h b/tools/net/ynl/generated/devlink-user.h >> index 1db4edc36eaa..e5d79b824a67 100644 >> --- a/tools/net/ynl/generated/devlink-user.h >> +++ b/tools/net/ynl/generated/devlink-user.h >> @@ -5252,4 +5252,51 @@ devlink_selftests_run_req_set_selftests_flash(struct devlink_selftests_run_req * >> int devlink_selftests_run(struct ynl_sock *ys, >> struct devlink_selftests_run_req *req); >> +/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */ >> +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */ >> +struct devlink_notify_filter_set_req { >> + struct { >> + __u32 bus_name_len; >> + __u32 dev_name_len; >> + } _present; >> + >> + char *bus_name; >> + char *dev_name; >> +}; >> + >> +static inline struct devlink_notify_filter_set_req * >> +devlink_notify_filter_set_req_alloc(void) >> +{ >> + return calloc(1, sizeof(struct devlink_notify_filter_set_req)); >> +} >> +void >> +devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req); >> + >> +static inline void >> +devlink_notify_filter_set_req_set_bus_name(struct devlink_notify_filter_set_req *req, >> + const char *bus_name) >> +{ >> + free(req->bus_name); >> + req->_present.bus_name_len = strlen(bus_name); >> + req->bus_name = malloc(req->_present.bus_name_len + 1); >> + memcpy(req->bus_name, bus_name, req->_present.bus_name_len); >> + req->bus_name[req->_present.bus_name_len] = 0; >> +} >> +static inline void >> +devlink_notify_filter_set_req_set_dev_name(struct devlink_notify_filter_set_req *req, >> + const char *dev_name) >> +{ >> + free(req->dev_name); >> + req->_present.dev_name_len = strlen(dev_name); >> + req->dev_name = malloc(req->_present.dev_name_len + 1); >> + memcpy(req->dev_name, dev_name, req->_present.dev_name_len); >> + req->dev_name[req->_present.dev_name_len] = 0; >> +} >> + >> +/* >> + * Set notification messages socket filter. >> + */ >> +int devlink_notify_filter_set(struct ynl_sock *ys, >> + struct devlink_notify_filter_set_req *req); >> + >> #endif /* _LINUX_DEVLINK_GEN_H */ >
Mon, Nov 27, 2023 at 04:40:22PM CET, przemyslaw.kitszel@intel.com wrote: >On 11/23/23 19:15, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@nvidia.com> >> [...] >> diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c >> index fa9afe3e6d9b..33a8e51dea68 100644 >> --- a/net/devlink/netlink.c >> +++ b/net/devlink/netlink.c >> @@ -17,6 +17,79 @@ static const struct genl_multicast_group devlink_nl_mcgrps[] = { >> [DEVLINK_MCGRP_CONFIG] = { .name = DEVLINK_GENL_MCGRP_CONFIG_NAME }, >> }; >> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb, >> + struct genl_info *info) >> +{ >> + struct nlattr **attrs = info->attrs; >> + struct devlink_obj_desc *flt; >> + size_t data_offset = 0; >> + size_t data_size = 0; >> + char *pos; >> + >> + if (attrs[DEVLINK_ATTR_BUS_NAME]) >> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1; >> + if (attrs[DEVLINK_ATTR_DEV_NAME]) >> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1; >> + >> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL); > >instead of arithmetic here, you could use struct_size() That is used for flex array, yet I have no flex array here. > >> + if (!flt) >> + return -ENOMEM; >> + >> + pos = (char *) flt->data; >> + if (attrs[DEVLINK_ATTR_BUS_NAME]) { >> + data_offset += nla_strscpy(pos, >> + attrs[DEVLINK_ATTR_BUS_NAME], >> + data_size) + 1; >> + flt->bus_name = pos; >> + pos += data_offset; >> + } >> + if (attrs[DEVLINK_ATTR_DEV_NAME]) { >> + nla_strscpy(pos, attrs[DEVLINK_ATTR_DEV_NAME], >> + data_size - data_offset); >> + flt->dev_name = pos; >> + } >> + >> + /* Don't attach empty filter. */ >> + if (!flt->bus_name && !flt->dev_name) { >> + kfree(flt); >> + flt = NULL; >> + } >> + > [...]
On Mon, Dec 04, 2023 at 05:24:34PM +0100, Jiri Pirko wrote: > Mon, Nov 27, 2023 at 04:40:22PM CET, przemyslaw.kitszel@intel.com wrote: > >On 11/23/23 19:15, Jiri Pirko wrote: [...] > >> + size_t data_size = 0; > >> + char *pos; > >> + > >> + if (attrs[DEVLINK_ATTR_BUS_NAME]) > >> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1; > >> + if (attrs[DEVLINK_ATTR_DEV_NAME]) > >> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1; > >> + > >> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL); > > > >instead of arithmetic here, you could use struct_size() > > That is used for flex array, yet I have no flex array here. It feels like you use flexible array even if you have the top limit of the size. But yeah, the attributes seem out of the variadic length and struct_size() here won't help anyway.
> -----Original Message----- > From: Jiri Pirko <jiri@resnulli.us> > Sent: Monday, December 4, 2023 8:25 AM > To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> > Cc: kuba@kernel.org; pabeni@redhat.com; davem@davemloft.net; > edumazet@google.com; Keller, Jacob E <jacob.e.keller@intel.com>; Hadi Salim, > Jamal <jhs@mojatatu.com>; johannes@sipsolutions.net; > andriy.shevchenko@linux.intel.com; Nambiar, Amritha > <amritha.nambiar@intel.com>; sdf@google.com; horms@kernel.org; > netdev@vger.kernel.org > Subject: Re: [patch net-next v4 8/9] devlink: add a command to set notification > filter and use it for multicasts > > Mon, Nov 27, 2023 at 04:40:22PM CET, przemyslaw.kitszel@intel.com wrote: > >On 11/23/23 19:15, Jiri Pirko wrote: > >> From: Jiri Pirko <jiri@nvidia.com> > >> > > [...] > > > >> diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c > >> index fa9afe3e6d9b..33a8e51dea68 100644 > >> --- a/net/devlink/netlink.c > >> +++ b/net/devlink/netlink.c > >> @@ -17,6 +17,79 @@ static const struct genl_multicast_group > devlink_nl_mcgrps[] = { > >> [DEVLINK_MCGRP_CONFIG] = { .name = > DEVLINK_GENL_MCGRP_CONFIG_NAME }, > >> }; > >> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb, > >> + struct genl_info *info) > >> +{ > >> + struct nlattr **attrs = info->attrs; > >> + struct devlink_obj_desc *flt; > >> + size_t data_offset = 0; > >> + size_t data_size = 0; > >> + char *pos; > >> + > >> + if (attrs[DEVLINK_ATTR_BUS_NAME]) > >> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1; > >> + if (attrs[DEVLINK_ATTR_DEV_NAME]) > >> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1; > >> + > >> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL); > > > >instead of arithmetic here, you could use struct_size() > > That is used for flex array, yet I have no flex array here. > Yea this isn't a flexible array. You could use size_add to ensure that this can't overflow. I don't know what the bound on the attribute sizes is.
Mon, Dec 04, 2023 at 08:17:24PM CET, jacob.e.keller@intel.com wrote: > > >> -----Original Message----- >> From: Jiri Pirko <jiri@resnulli.us> >> Sent: Monday, December 4, 2023 8:25 AM >> To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> >> Cc: kuba@kernel.org; pabeni@redhat.com; davem@davemloft.net; >> edumazet@google.com; Keller, Jacob E <jacob.e.keller@intel.com>; Hadi Salim, >> Jamal <jhs@mojatatu.com>; johannes@sipsolutions.net; >> andriy.shevchenko@linux.intel.com; Nambiar, Amritha >> <amritha.nambiar@intel.com>; sdf@google.com; horms@kernel.org; >> netdev@vger.kernel.org >> Subject: Re: [patch net-next v4 8/9] devlink: add a command to set notification >> filter and use it for multicasts >> >> Mon, Nov 27, 2023 at 04:40:22PM CET, przemyslaw.kitszel@intel.com wrote: >> >On 11/23/23 19:15, Jiri Pirko wrote: >> >> From: Jiri Pirko <jiri@nvidia.com> >> >> >> >> [...] >> >> >> >> diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c >> >> index fa9afe3e6d9b..33a8e51dea68 100644 >> >> --- a/net/devlink/netlink.c >> >> +++ b/net/devlink/netlink.c >> >> @@ -17,6 +17,79 @@ static const struct genl_multicast_group >> devlink_nl_mcgrps[] = { >> >> [DEVLINK_MCGRP_CONFIG] = { .name = >> DEVLINK_GENL_MCGRP_CONFIG_NAME }, >> >> }; >> >> +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb, >> >> + struct genl_info *info) >> >> +{ >> >> + struct nlattr **attrs = info->attrs; >> >> + struct devlink_obj_desc *flt; >> >> + size_t data_offset = 0; >> >> + size_t data_size = 0; >> >> + char *pos; >> >> + >> >> + if (attrs[DEVLINK_ATTR_BUS_NAME]) >> >> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1; >> >> + if (attrs[DEVLINK_ATTR_DEV_NAME]) >> >> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1; >> >> + >> >> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL); >> > >> >instead of arithmetic here, you could use struct_size() >> >> That is used for flex array, yet I have no flex array here. >> > >Yea this isn't a flexible array. You could use size_add to ensure that this can't overflow. I don't know what the bound on the attribute sizes is. Okay, will do that to be on a safe side. >
On Tue, Dec 05, 2023 at 08:47:43AM +0100, Jiri Pirko wrote: > Mon, Dec 04, 2023 at 08:17:24PM CET, jacob.e.keller@intel.com wrote: > >> -----Original Message----- > >> From: Jiri Pirko <jiri@resnulli.us> > >> Sent: Monday, December 4, 2023 8:25 AM > >> Mon, Nov 27, 2023 at 04:40:22PM CET, przemyslaw.kitszel@intel.com wrote: > >> >On 11/23/23 19:15, Jiri Pirko wrote: ... > >> >> + if (attrs[DEVLINK_ATTR_BUS_NAME]) > >> >> + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1; > >> >> + if (attrs[DEVLINK_ATTR_DEV_NAME]) > >> >> + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1; > >> >> + > >> >> + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL); > >> > > >> >instead of arithmetic here, you could use struct_size() > >> > >> That is used for flex array, yet I have no flex array here. > > > >Yea this isn't a flexible array. You could use size_add to ensure that this > >can't overflow. I don't know what the bound on the attribute sizes is. > > Okay, will do that to be on a safe side. If we go this direction it may be makes sense to have done inside nla_len():ish type of helper, so it will be once for everyone. But I haven't checked the code on how many cases we have when we need to count the size depending on the present attributes.
diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml index 43067e1f63aa..6bad1d3454b7 100644 --- a/Documentation/netlink/specs/devlink.yaml +++ b/Documentation/netlink/specs/devlink.yaml @@ -2055,3 +2055,13 @@ operations: - bus-name - dev-name - selftests + + - + name: notify-filter-set + doc: Set notification messages socket filter. + attribute-set: devlink + do: + request: + attributes: + - bus-name + - dev-name diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index b3c8383d342d..130cae0d3e20 100644 --- a/include/uapi/linux/devlink.h +++ b/include/uapi/linux/devlink.h @@ -139,6 +139,8 @@ enum devlink_command { DEVLINK_CMD_SELFTESTS_GET, /* can dump */ DEVLINK_CMD_SELFTESTS_RUN, + DEVLINK_CMD_NOTIFY_FILTER_SET, + /* add new commands above here */ __DEVLINK_CMD_MAX, DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1 diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h index 84dc9628d3f2..82e0fb3bbebf 100644 --- a/net/devlink/devl_internal.h +++ b/net/devlink/devl_internal.h @@ -191,11 +191,41 @@ static inline bool devlink_nl_notify_need(struct devlink *devlink) DEVLINK_MCGRP_CONFIG); } +struct devlink_obj_desc { + struct rcu_head rcu; + const char *bus_name; + const char *dev_name; + long data[]; +}; + +static inline void devlink_nl_obj_desc_init(struct devlink_obj_desc *desc, + struct devlink *devlink) +{ + memset(desc, 0, sizeof(*desc)); + desc->bus_name = devlink->dev->bus->name; + desc->dev_name = dev_name(devlink->dev); +} + +int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data); + +static inline void devlink_nl_notify_send_desc(struct devlink *devlink, + struct sk_buff *msg, + struct devlink_obj_desc *desc) +{ + genlmsg_multicast_netns_filtered(&devlink_nl_family, + devlink_net(devlink), + msg, 0, DEVLINK_MCGRP_CONFIG, + GFP_KERNEL, + devlink_nl_notify_filter, desc); +} + static inline void devlink_nl_notify_send(struct devlink *devlink, struct sk_buff *msg) { - genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink), - msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL); + struct devlink_obj_desc desc; + + devlink_nl_obj_desc_init(&desc, devlink); + devlink_nl_notify_send_desc(devlink, msg, &desc); } /* Notify */ diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c index fa9afe3e6d9b..33a8e51dea68 100644 --- a/net/devlink/netlink.c +++ b/net/devlink/netlink.c @@ -17,6 +17,79 @@ static const struct genl_multicast_group devlink_nl_mcgrps[] = { [DEVLINK_MCGRP_CONFIG] = { .name = DEVLINK_GENL_MCGRP_CONFIG_NAME }, }; +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb, + struct genl_info *info) +{ + struct nlattr **attrs = info->attrs; + struct devlink_obj_desc *flt; + size_t data_offset = 0; + size_t data_size = 0; + char *pos; + + if (attrs[DEVLINK_ATTR_BUS_NAME]) + data_size += nla_len(attrs[DEVLINK_ATTR_BUS_NAME]) + 1; + if (attrs[DEVLINK_ATTR_DEV_NAME]) + data_size += nla_len(attrs[DEVLINK_ATTR_DEV_NAME]) + 1; + + flt = kzalloc(sizeof(*flt) + data_size, GFP_KERNEL); + if (!flt) + return -ENOMEM; + + pos = (char *) flt->data; + if (attrs[DEVLINK_ATTR_BUS_NAME]) { + data_offset += nla_strscpy(pos, + attrs[DEVLINK_ATTR_BUS_NAME], + data_size) + 1; + flt->bus_name = pos; + pos += data_offset; + } + if (attrs[DEVLINK_ATTR_DEV_NAME]) { + nla_strscpy(pos, attrs[DEVLINK_ATTR_DEV_NAME], + data_size - data_offset); + flt->dev_name = pos; + } + + /* Don't attach empty filter. */ + if (!flt->bus_name && !flt->dev_name) { + kfree(flt); + flt = NULL; + } + + flt = genl_sk_priv_store(NETLINK_CB(skb).sk, &devlink_nl_family, flt); + if (IS_ERR(flt)) + return PTR_ERR(flt); + else if (flt) + kfree_rcu(flt, rcu); + + return 0; +} + +static bool devlink_obj_desc_match(const struct devlink_obj_desc *desc, + const struct devlink_obj_desc *flt) +{ + if (desc->bus_name && flt->bus_name && + strcmp(desc->bus_name, flt->bus_name)) + return false; + if (desc->dev_name && flt->dev_name && + strcmp(desc->dev_name, flt->dev_name)) + return false; + return true; +} + +int devlink_nl_notify_filter(struct sock *dsk, struct sk_buff *skb, void *data) +{ + struct devlink_obj_desc *desc = data; + struct devlink_obj_desc *flt; + int ret = 0; + + rcu_read_lock(); + flt = genl_sk_priv_get(dsk, &devlink_nl_family); + if (flt) + ret = !devlink_obj_desc_match(desc, flt); + rcu_read_unlock(); + return ret; +} + int devlink_nl_put_nested_handle(struct sk_buff *msg, struct net *net, struct devlink *devlink, int attrtype) { diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c index 95f9b4350ab7..1cb0e05305d2 100644 --- a/net/devlink/netlink_gen.c +++ b/net/devlink/netlink_gen.c @@ -560,8 +560,14 @@ static const struct nla_policy devlink_selftests_run_nl_policy[DEVLINK_ATTR_SELF [DEVLINK_ATTR_SELFTESTS] = NLA_POLICY_NESTED(devlink_dl_selftest_id_nl_policy), }; +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */ +static const struct nla_policy devlink_notify_filter_set_nl_policy[DEVLINK_ATTR_DEV_NAME + 1] = { + [DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING, }, + [DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, }, +}; + /* Ops table for devlink */ -const struct genl_split_ops devlink_nl_ops[73] = { +const struct genl_split_ops devlink_nl_ops[74] = { { .cmd = DEVLINK_CMD_GET, .validate = GENL_DONT_VALIDATE_STRICT, @@ -1233,4 +1239,11 @@ const struct genl_split_ops devlink_nl_ops[73] = { .maxattr = DEVLINK_ATTR_SELFTESTS, .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, }, + { + .cmd = DEVLINK_CMD_NOTIFY_FILTER_SET, + .doit = devlink_nl_notify_filter_set_doit, + .policy = devlink_notify_filter_set_nl_policy, + .maxattr = DEVLINK_ATTR_DEV_NAME, + .flags = GENL_CMD_CAP_DO, + }, }; diff --git a/net/devlink/netlink_gen.h b/net/devlink/netlink_gen.h index 02f3c0bfae0e..8f2bd50ddf5e 100644 --- a/net/devlink/netlink_gen.h +++ b/net/devlink/netlink_gen.h @@ -16,7 +16,7 @@ extern const struct nla_policy devlink_dl_port_function_nl_policy[DEVLINK_PORT_F extern const struct nla_policy devlink_dl_selftest_id_nl_policy[DEVLINK_ATTR_SELFTEST_ID_FLASH + 1]; /* Ops table for devlink */ -extern const struct genl_split_ops devlink_nl_ops[73]; +extern const struct genl_split_ops devlink_nl_ops[74]; int devlink_nl_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, struct genl_info *info); @@ -142,5 +142,7 @@ int devlink_nl_selftests_get_doit(struct sk_buff *skb, struct genl_info *info); int devlink_nl_selftests_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb); int devlink_nl_selftests_run_doit(struct sk_buff *skb, struct genl_info *info); +int devlink_nl_notify_filter_set_doit(struct sk_buff *skb, + struct genl_info *info); #endif /* _LINUX_DEVLINK_GEN_H */ diff --git a/tools/net/ynl/generated/devlink-user.c b/tools/net/ynl/generated/devlink-user.c index bc5065bd99b2..cd5f70eadf5b 100644 --- a/tools/net/ynl/generated/devlink-user.c +++ b/tools/net/ynl/generated/devlink-user.c @@ -6830,6 +6830,37 @@ int devlink_selftests_run(struct ynl_sock *ys, return 0; } +/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */ +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */ +void +devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req) +{ + free(req->bus_name); + free(req->dev_name); + free(req); +} + +int devlink_notify_filter_set(struct ynl_sock *ys, + struct devlink_notify_filter_set_req *req) +{ + struct nlmsghdr *nlh; + int err; + + nlh = ynl_gemsg_start_req(ys, ys->family_id, DEVLINK_CMD_NOTIFY_FILTER_SET, 1); + ys->req_policy = &devlink_nest; + + if (req->_present.bus_name_len) + mnl_attr_put_strz(nlh, DEVLINK_ATTR_BUS_NAME, req->bus_name); + if (req->_present.dev_name_len) + mnl_attr_put_strz(nlh, DEVLINK_ATTR_DEV_NAME, req->dev_name); + + err = ynl_exec(ys, nlh, NULL); + if (err < 0) + return -1; + + return 0; +} + const struct ynl_family ynl_devlink_family = { .name = "devlink", }; diff --git a/tools/net/ynl/generated/devlink-user.h b/tools/net/ynl/generated/devlink-user.h index 1db4edc36eaa..e5d79b824a67 100644 --- a/tools/net/ynl/generated/devlink-user.h +++ b/tools/net/ynl/generated/devlink-user.h @@ -5252,4 +5252,51 @@ devlink_selftests_run_req_set_selftests_flash(struct devlink_selftests_run_req * int devlink_selftests_run(struct ynl_sock *ys, struct devlink_selftests_run_req *req); +/* ============== DEVLINK_CMD_NOTIFY_FILTER_SET ============== */ +/* DEVLINK_CMD_NOTIFY_FILTER_SET - do */ +struct devlink_notify_filter_set_req { + struct { + __u32 bus_name_len; + __u32 dev_name_len; + } _present; + + char *bus_name; + char *dev_name; +}; + +static inline struct devlink_notify_filter_set_req * +devlink_notify_filter_set_req_alloc(void) +{ + return calloc(1, sizeof(struct devlink_notify_filter_set_req)); +} +void +devlink_notify_filter_set_req_free(struct devlink_notify_filter_set_req *req); + +static inline void +devlink_notify_filter_set_req_set_bus_name(struct devlink_notify_filter_set_req *req, + const char *bus_name) +{ + free(req->bus_name); + req->_present.bus_name_len = strlen(bus_name); + req->bus_name = malloc(req->_present.bus_name_len + 1); + memcpy(req->bus_name, bus_name, req->_present.bus_name_len); + req->bus_name[req->_present.bus_name_len] = 0; +} +static inline void +devlink_notify_filter_set_req_set_dev_name(struct devlink_notify_filter_set_req *req, + const char *dev_name) +{ + free(req->dev_name); + req->_present.dev_name_len = strlen(dev_name); + req->dev_name = malloc(req->_present.dev_name_len + 1); + memcpy(req->dev_name, dev_name, req->_present.dev_name_len); + req->dev_name[req->_present.dev_name_len] = 0; +} + +/* + * Set notification messages socket filter. + */ +int devlink_notify_filter_set(struct ynl_sock *ys, + struct devlink_notify_filter_set_req *req); + #endif /* _LINUX_DEVLINK_GEN_H */