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(-)
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)
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.
linux-stable-mirror@lists.linaro.org
-
Jakub Kicinski -
Joe Damato