| Message ID | 20241112181401.9689-2-jdamato@fastly.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix rcu_read_lock issues in netdev-genl | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/apply | fail | Patch does not apply to net-0 |
On Tue, 12 Nov 2024 18:13:58 +0000 Joe Damato wrote: > +/* must be called under rcu_read_lock(), because napi_by_id requires it */ > +static struct napi_struct *__do_napi_by_id(unsigned int napi_id, > + struct genl_info *info, int *err) > +{ > + struct napi_struct *napi; > + > + napi = napi_by_id(napi_id); > + if (napi) { > + *err = 0; > + } else { > + NL_SET_BAD_ATTR(info->extack, info->attrs[NETDEV_A_NAPI_ID]); > + *err = -ENOENT; > + } > + > + return napi; > +} Thanks for the quick follow up! I vote we don't factor this out. I don't see what it buys us, TBH, normally we factor out code to avoid having to unlock before return, but this code doesn't have extra returns... Just slap an rcu_read_lock / unlock around and that's it? Feel free to repost soon.
On Tue, Nov 12, 2024 at 05:28:40PM -0800, Jakub Kicinski wrote: > On Tue, 12 Nov 2024 18:13:58 +0000 Joe Damato wrote: > > +/* must be called under rcu_read_lock(), because napi_by_id requires it */ > > +static struct napi_struct *__do_napi_by_id(unsigned int napi_id, > > + struct genl_info *info, int *err) > > +{ > > + struct napi_struct *napi; > > + > > + napi = napi_by_id(napi_id); > > + if (napi) { > > + *err = 0; > > + } else { > > + NL_SET_BAD_ATTR(info->extack, info->attrs[NETDEV_A_NAPI_ID]); > > + *err = -ENOENT; > > + } > > + > > + return napi; > > +} > > Thanks for the quick follow up! I vote we don't factor this out. > I don't see what it buys us, TBH, normally we factor out code > to avoid having to unlock before return, but this code doesn't > have extra returns... > > Just slap an rcu_read_lock / unlock around and that's it? Sure sounds good. Sorry for the noob question: should I break it up into two patches with one CCing stable and the other not like I did for this RFC? Patch 1 definitely "feels" like a fixes + CC stable Patch 2 could be either net-next or a net + "fixes" without stable? > Feel free to repost soon. Will do, just lmk on the above so I can submit it the correct way. Thanks for the quick feedback.
On Tue, 12 Nov 2024 17:48:42 -0800 Joe Damato wrote: > Sorry for the noob question: should I break it up into two patches > with one CCing stable and the other not like I did for this RFC? > > Patch 1 definitely "feels" like a fixes + CC stable > Patch 2 could be either net-next or a net + "fixes" without stable? Oh, sorry, I didn't comment on that because that part is correct. The split is great, will make backporting easier.
On Tue, Nov 12, 2024 at 06:01:02PM -0800, Jakub Kicinski wrote: > On Tue, 12 Nov 2024 17:48:42 -0800 Joe Damato wrote: > > Sorry for the noob question: should I break it up into two patches > > with one CCing stable and the other not like I did for this RFC? > > > > Patch 1 definitely "feels" like a fixes + CC stable > > Patch 2 could be either net-next or a net + "fixes" without stable? > > Oh, sorry, I didn't comment on that because that part is correct. > The split is great, will make backporting easier. OK, cool, that's what I figured. Thanks for the guidance; will repost shortly.
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c index 765ce7c9d73b..934c63a93524 100644 --- a/net/core/netdev-genl.c +++ b/net/core/netdev-genl.c @@ -216,6 +216,23 @@ netdev_nl_napi_fill_one(struct sk_buff *rsp, struct napi_struct *napi, return -EMSGSIZE; } +/* must be called under rcu_read_lock(), because napi_by_id requires it */ +static struct napi_struct *__do_napi_by_id(unsigned int napi_id, + struct genl_info *info, int *err) +{ + struct napi_struct *napi; + + napi = napi_by_id(napi_id); + if (napi) { + *err = 0; + } else { + NL_SET_BAD_ATTR(info->extack, info->attrs[NETDEV_A_NAPI_ID]); + *err = -ENOENT; + } + + return napi; +} + int netdev_nl_napi_get_doit(struct sk_buff *skb, struct genl_info *info) { struct napi_struct *napi; @@ -233,15 +250,13 @@ int netdev_nl_napi_get_doit(struct sk_buff *skb, struct genl_info *info) return -ENOMEM; rtnl_lock(); + rcu_read_lock(); - napi = napi_by_id(napi_id); - if (napi) { + napi = __do_napi_by_id(napi_id, info, &err); + if (!err) err = netdev_nl_napi_fill_one(rsp, napi, info); - } else { - NL_SET_BAD_ATTR(info->extack, info->attrs[NETDEV_A_NAPI_ID]); - err = -ENOENT; - } + rcu_read_unlock(); rtnl_unlock(); if (err)
Hold rcu_read_lock in netdev_nl_napi_get_doit, which calls napi_by_id and is required to be called under rcu_read_lock. Cc: stable@vger.kernel.org Fixes: 27f91aaf49b3 ("netdev-genl: Add netlink framework functions for napi") Signed-off-by: Joe Damato <jdamato@fastly.com> --- net/core/netdev-genl.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)