[net v2 0/2] Fix rcu_read_lock issues in netdev-genl

[net v2 0/2] Fix rcu_read_lock issues in netdev-genl

[Joe Damato]

Greetings:

Paolo reported a splat [1] when running the new selftest for busy poll.
I confirmed and reproduced this splat locally.

This series proposes 2 patches:
  - Patch 1:
    - Fixes a similar issue in an older commit and CCs stable as this
      fix could be backported.
  - Patch 2:
    - Fixes the issue Paolo hit while running the selftest

I retested locally after applying this series and confirmed that the
splat is fixed.

Thanks,
Joe

[1]: https://lore.kernel.org/netdev/719083c2-e277-447b-b6ea-ca3acb293a03@redhat.com/

v2:
  - Removed the helper and simplified to just add a rcu_read_lock /
    unlock in both patches instead.

rfc: https://lore.kernel.org/lkml/20241112181401.9689-1-jdamato@fastly.com/

Joe Damato (2):
  netdev-genl: Hold rcu_read_lock in napi_get
  netdev-genl: Hold rcu_read_lock in napi_set

 net/core/netdev-genl.c | 4 ++++
 1 file changed, 4 insertions(+)


base-commit: a58f00ed24b849d449f7134fd5d86f07090fe2f5
-- 
2.25.1

[net v2 1/2] netdev-genl: Hold rcu_read_lock in napi_get

[Joe Damato]

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>
---
 v2:
   - Simplified by removing the helper and calling rcu_read_lock /
     unlock directly instead.

 net/core/netdev-genl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 765ce7c9d73b..0b684410b52d 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -233,6 +233,7 @@ 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) {
@@ -242,6 +243,7 @@ int netdev_nl_napi_get_doit(struct sk_buff *skb, struct genl_info *info)
 		err = -ENOENT;
 	}
 
+	rcu_read_unlock();
 	rtnl_unlock();
 
 	if (err)
-- 
2.25.1

[net v2 2/2] netdev-genl: Hold rcu_read_lock in napi_set

[Joe Damato]

Hold rcu_read_lock during netdev_nl_napi_set_doit, which calls
napi_by_id and requires rcu_read_lock to be held.

Closes: https://lore.kernel.org/netdev/719083c2-e277-447b-b6ea-ca3acb293a03@redhat.com/
Fixes: 1287c1ae0fc2 ("netdev-genl: Support setting per-NAPI config values")
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 v2:
   - Simplified by adding rcu_read_lock/unlock instead of using the
     helper proposed in the RFC.

 net/core/netdev-genl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 0b684410b52d..9527dd46e4dc 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -348,6 +348,7 @@ int netdev_nl_napi_set_doit(struct sk_buff *skb, struct genl_info *info)
 	napi_id = nla_get_u32(info->attrs[NETDEV_A_NAPI_ID]);
 
 	rtnl_lock();
+	rcu_read_lock();
 
 	napi = napi_by_id(napi_id);
 	if (napi) {
@@ -357,6 +358,7 @@ int netdev_nl_napi_set_doit(struct sk_buff *skb, struct genl_info *info)
 		err = -ENOENT;
 	}
 
+	rcu_read_unlock();
 	rtnl_unlock();
 
 	return err;
-- 
2.25.1

Re: [net v2 0/2] Fix rcu_read_lock issues in netdev-genl

[Jakub Kicinski]

On Wed, 13 Nov 2024 02:17:50 +0000 Joe Damato wrote:
> base-commit: a58f00ed24b849d449f7134fd5d86f07090fe2f5

which is a net-next commit.. please rebase on net

Re: [net v2 0/2] Fix rcu_read_lock issues in netdev-genl

[Joe Damato]

On Wed, Nov 13, 2024 at 06:47:35PM -0800, Jakub Kicinski wrote:
> On Wed, 13 Nov 2024 02:17:50 +0000 Joe Damato wrote:
> > base-commit: a58f00ed24b849d449f7134fd5d86f07090fe2f5
> 
> which is a net-next commit.. please rebase on net

I thought I asked about this in the previous thread, but I probably
wasn't clear with my question.

Let me try again:

Patch 1 will apply to net and is a fixes and CC's stable, and fixes
a similar issue to the one Paolo reported, not the exact same path,
though.

Patch 2 will not apply to net, because the code it fixes is not in
net yet. This fixes the splat Paolo reported.

So... back to the question in the cover letter from the RFC :) I
suppose the right thing to do is split the series:

- Rebase patch 1 on net (it applies as is) and send it on its own
- Send patch 2 on its own against net-next

Or... something else ?

Re: [net v2 0/2] Fix rcu_read_lock issues in netdev-genl

[Paolo Abeni]

On 11/14/24 07:29, Joe Damato wrote:
> On Wed, Nov 13, 2024 at 06:47:35PM -0800, Jakub Kicinski wrote:
>> On Wed, 13 Nov 2024 02:17:50 +0000 Joe Damato wrote:
>>> base-commit: a58f00ed24b849d449f7134fd5d86f07090fe2f5
>>
>> which is a net-next commit.. please rebase on net
> 
> I thought I asked about this in the previous thread, but I probably
> wasn't clear with my question.
> 
> Let me try again:
> 
> Patch 1 will apply to net and is a fixes and CC's stable, and fixes
> a similar issue to the one Paolo reported, not the exact same path,
> though.
> 
> Patch 2 will not apply to net, because the code it fixes is not in
> net yet. This fixes the splat Paolo reported.
> 
> So... back to the question in the cover letter from the RFC :) I
> suppose the right thing to do is split the series:
> 
> - Rebase patch 1 on net (it applies as is) and send it on its own
> - Send patch 2 on its own against net-next
> 
> Or... something else ?

I'm sorry for the late reply.

Please send the two patch separately, patch 1 targeting (and rebased on)
net and patch 2 targeting (and based on) net-next.

Thanks!

Paolo

Re: [net v2 0/2] Fix rcu_read_lock issues in netdev-genl

[Joe Damato]

On Thu, Nov 14, 2024 at 10:06:02AM +0100, Paolo Abeni wrote:
> 
> 
> On 11/14/24 07:29, Joe Damato wrote:
> > On Wed, Nov 13, 2024 at 06:47:35PM -0800, Jakub Kicinski wrote:
> >> On Wed, 13 Nov 2024 02:17:50 +0000 Joe Damato wrote:
> >>> base-commit: a58f00ed24b849d449f7134fd5d86f07090fe2f5
> >>
> >> which is a net-next commit.. please rebase on net
> > 
> > I thought I asked about this in the previous thread, but I probably
> > wasn't clear with my question.
> > 
> > Let me try again:
> > 
> > Patch 1 will apply to net and is a fixes and CC's stable, and fixes
> > a similar issue to the one Paolo reported, not the exact same path,
> > though.
> > 
> > Patch 2 will not apply to net, because the code it fixes is not in
> > net yet. This fixes the splat Paolo reported.
> > 
> > So... back to the question in the cover letter from the RFC :) I
> > suppose the right thing to do is split the series:
> > 
> > - Rebase patch 1 on net (it applies as is) and send it on its own
> > - Send patch 2 on its own against net-next
> > 
> > Or... something else ?
> 
> I'm sorry for the late reply.
> 
> Please send the two patch separately, patch 1 targeting (and rebased on)
> net and patch 2 targeting (and based on) net-next.
> 

OK, I've done that. I left the fixes tag on patch 2 despite it
targeting net-next, but didn't CC stable since the code doesn't need
to be backported.

Re: [net v2 0/2] Fix rcu_read_lock issues in netdev-genl

[Paolo Abeni]

On 11/14/24 18:58, Joe Damato wrote:
> On Thu, Nov 14, 2024 at 10:06:02AM +0100, Paolo Abeni wrote:
>> Please send the two patch separately, patch 1 targeting (and rebased on)
>> net and patch 2 targeting (and based on) net-next.
> 
> OK, I've done that. I left the fixes tag on patch 2 despite it
> targeting net-next, but didn't CC stable since the code doesn't need
> to be backported.

Thanks! FTR the above was the right thing to do.

/P

Re: [net v2 0/2] Fix rcu_read_lock issues in netdev-genl

[Jakub Kicinski]

On Wed, 13 Nov 2024 22:29:50 -0800 Joe Damato wrote:
> - Rebase patch 1 on net (it applies as is) and send it on its own
> - Send patch 2 on its own against net-next

My bad, I thought patch 2 is also needed in net, but not in stable.

Re: [net v2 0/2] Fix rcu_read_lock issues in netdev-genl

[Joe Damato]

On Thu, Nov 14, 2024 at 11:31:44AM -0800, Jakub Kicinski wrote:
> On Wed, 13 Nov 2024 22:29:50 -0800 Joe Damato wrote:
> > - Rebase patch 1 on net (it applies as is) and send it on its own
> > - Send patch 2 on its own against net-next
> 
> My bad, I thought patch 2 is also needed in net, but not in stable.

No problem; sorry for the noob confusing on my side. Hopefully, I
got it right for the v3.