* [PATCH net-next] net-sysfs: remove possible sleep from an RCU read-side critical section
@ 2021-03-22 15:43 Antoine Tenart
2021-03-22 16:54 ` Matthew Wilcox
2021-03-22 20:40 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 7+ messages in thread
From: Antoine Tenart @ 2021-03-22 15:43 UTC (permalink / raw)
To: davem, kuba, alexander.duyck
Cc: Antoine Tenart, netdev, kernel test robot, Matthew Wilcox
xps_queue_show is mostly made of an RCU read-side critical section and
calls bitmap_zalloc with GFP_KERNEL in the middle of it. That is not
allowed as this call may sleep and such behaviours aren't allowed in RCU
read-side critical sections. Fix this by using GFP_NOWAIT instead.
Fixes: 5478fcd0f483 ("net: embed nr_ids in the xps maps")
Reported-by: kernel test robot <oliver.sang@intel.com>
Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
Fix sent to net-next as it fixes an issue only in net-next.
net/core/net-sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 562a42fcd437..f6197774048b 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1378,7 +1378,7 @@ static ssize_t xps_queue_show(struct net_device *dev, unsigned int index,
nr_ids = dev_maps ? dev_maps->nr_ids :
(type == XPS_CPUS ? nr_cpu_ids : dev->num_rx_queues);
- mask = bitmap_zalloc(nr_ids, GFP_KERNEL);
+ mask = bitmap_zalloc(nr_ids, GFP_NOWAIT);
if (!mask) {
rcu_read_unlock();
return -ENOMEM;
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net-sysfs: remove possible sleep from an RCU read-side critical section
2021-03-22 15:43 [PATCH net-next] net-sysfs: remove possible sleep from an RCU read-side critical section Antoine Tenart
@ 2021-03-22 16:54 ` Matthew Wilcox
2021-03-22 17:41 ` Antoine Tenart
2021-03-22 20:40 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 7+ messages in thread
From: Matthew Wilcox @ 2021-03-22 16:54 UTC (permalink / raw)
To: Antoine Tenart; +Cc: davem, kuba, alexander.duyck, netdev, kernel test robot
On Mon, Mar 22, 2021 at 04:43:29PM +0100, Antoine Tenart wrote:
> xps_queue_show is mostly made of an RCU read-side critical section and
> calls bitmap_zalloc with GFP_KERNEL in the middle of it. That is not
> allowed as this call may sleep and such behaviours aren't allowed in RCU
> read-side critical sections. Fix this by using GFP_NOWAIT instead.
This would be another way of fixing the problem that is slightly less
complex than my initial proposal, but does allow for using GFP_KERNEL
for fewer failures:
@@ -1366,11 +1366,10 @@ static ssize_t xps_queue_show(struct net_device *dev, unsigned int index,
{
struct xps_dev_maps *dev_maps;
unsigned long *mask;
- unsigned int nr_ids;
+ unsigned int nr_ids, new_nr_ids;
int j, len;
- rcu_read_lock();
- dev_maps = rcu_dereference(dev->xps_maps[type]);
+ dev_maps = READ_ONCE(dev->xps_maps[type]);
/* Default to nr_cpu_ids/dev->num_rx_queues and do not just return 0
* when dev_maps hasn't been allocated yet, to be backward compatible.
@@ -1379,10 +1378,18 @@ static ssize_t xps_queue_show(struct net_device *dev, unsigned int index,
(type == XPS_CPUS ? nr_cpu_ids : dev->num_rx_queues);
mask = bitmap_zalloc(nr_ids, GFP_KERNEL);
- if (!mask) {
- rcu_read_unlock();
+ if (!mask)
return -ENOMEM;
- }
+
+ rcu_read_lock();
+ dev_maps = rcu_dereference(dev->xps_maps[type]);
+ /* if nr_ids shrank in the meantime, do not overrun array.
+ * if it increased, we just won't show the new ones
+ */
+ new_nr_ids = dev_maps ? dev_maps->nr_ids :
+ (type == XPS_CPUS ? nr_cpu_ids : dev->num_rx_queues);
+ if (new_nr_ids < nr_ids)
+ nr_ids = new_nr_ids;
if (!dev_maps || tc >= dev_maps->num_tc)
goto out_no_maps;
(or do we need the rcu read lock to read dev->num_rcx_queues? i'm assuming
we only need it to read the xps_maps array)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net-sysfs: remove possible sleep from an RCU read-side critical section
2021-03-22 16:54 ` Matthew Wilcox
@ 2021-03-22 17:41 ` Antoine Tenart
2021-03-22 17:44 ` Matthew Wilcox
2021-03-22 17:45 ` Antoine Tenart
0 siblings, 2 replies; 7+ messages in thread
From: Antoine Tenart @ 2021-03-22 17:41 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: davem, kuba, alexander.duyck, netdev, kernel test robot
Quoting Matthew Wilcox (2021-03-22 17:54:39)
> On Mon, Mar 22, 2021 at 04:43:29PM +0100, Antoine Tenart wrote:
> > xps_queue_show is mostly made of an RCU read-side critical section and
> > calls bitmap_zalloc with GFP_KERNEL in the middle of it. That is not
> > allowed as this call may sleep and such behaviours aren't allowed in RCU
> > read-side critical sections. Fix this by using GFP_NOWAIT instead.
>
> This would be another way of fixing the problem that is slightly less
> complex than my initial proposal, but does allow for using GFP_KERNEL
> for fewer failures:
>
> @@ -1366,11 +1366,10 @@ static ssize_t xps_queue_show(struct net_device *dev, unsigned int index,
> {
> struct xps_dev_maps *dev_maps;
> unsigned long *mask;
> - unsigned int nr_ids;
> + unsigned int nr_ids, new_nr_ids;
> int j, len;
>
> - rcu_read_lock();
> - dev_maps = rcu_dereference(dev->xps_maps[type]);
> + dev_maps = READ_ONCE(dev->xps_maps[type]);
Couldn't dev_maps be freed between here and the read of dev_maps->nr_ids
as we're not in an RCU read-side critical section?
> /* Default to nr_cpu_ids/dev->num_rx_queues and do not just return 0
> * when dev_maps hasn't been allocated yet, to be backward compatible.
> @@ -1379,10 +1378,18 @@ static ssize_t xps_queue_show(struct net_device *dev, unsigned int index,
> (type == XPS_CPUS ? nr_cpu_ids : dev->num_rx_queues);
>
> mask = bitmap_zalloc(nr_ids, GFP_KERNEL);
> - if (!mask) {
> - rcu_read_unlock();
> + if (!mask)
> return -ENOMEM;
> - }
> +
> + rcu_read_lock();
> + dev_maps = rcu_dereference(dev->xps_maps[type]);
> + /* if nr_ids shrank in the meantime, do not overrun array.
> + * if it increased, we just won't show the new ones
> + */
> + new_nr_ids = dev_maps ? dev_maps->nr_ids :
> + (type == XPS_CPUS ? nr_cpu_ids : dev->num_rx_queues);
> + if (new_nr_ids < nr_ids)
> + nr_ids = new_nr_ids;
>
> if (!dev_maps || tc >= dev_maps->num_tc)
> goto out_no_maps;
My feeling is there is not much value in having a tricky allocation
logic for reads from xps_cpus and xps_rxqs. While we could come up with
something, returning -ENOMEM on memory pressure should be fine.
Antoine
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net-sysfs: remove possible sleep from an RCU read-side critical section
2021-03-22 17:41 ` Antoine Tenart
@ 2021-03-22 17:44 ` Matthew Wilcox
2021-03-22 17:49 ` Antoine Tenart
2021-03-22 17:45 ` Antoine Tenart
1 sibling, 1 reply; 7+ messages in thread
From: Matthew Wilcox @ 2021-03-22 17:44 UTC (permalink / raw)
To: Antoine Tenart; +Cc: davem, kuba, alexander.duyck, netdev, kernel test robot
On Mon, Mar 22, 2021 at 06:41:30PM +0100, Antoine Tenart wrote:
> Quoting Matthew Wilcox (2021-03-22 17:54:39)
> > - rcu_read_lock();
> > - dev_maps = rcu_dereference(dev->xps_maps[type]);
> > + dev_maps = READ_ONCE(dev->xps_maps[type]);
>
> Couldn't dev_maps be freed between here and the read of dev_maps->nr_ids
> as we're not in an RCU read-side critical section?
Oh, good point. Never mind, then.
> My feeling is there is not much value in having a tricky allocation
> logic for reads from xps_cpus and xps_rxqs. While we could come up with
> something, returning -ENOMEM on memory pressure should be fine.
That's fine. It's your code, and this is probably a small allocation
anyway.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net-sysfs: remove possible sleep from an RCU read-side critical section
2021-03-22 17:41 ` Antoine Tenart
2021-03-22 17:44 ` Matthew Wilcox
@ 2021-03-22 17:45 ` Antoine Tenart
1 sibling, 0 replies; 7+ messages in thread
From: Antoine Tenart @ 2021-03-22 17:45 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: davem, kuba, alexander.duyck, netdev, kernel test robot
Quoting Antoine Tenart (2021-03-22 18:41:30)
> Quoting Matthew Wilcox (2021-03-22 17:54:39)
> > On Mon, Mar 22, 2021 at 04:43:29PM +0100, Antoine Tenart wrote:
> > > xps_queue_show is mostly made of an RCU read-side critical section and
> > > calls bitmap_zalloc with GFP_KERNEL in the middle of it. That is not
> > > allowed as this call may sleep and such behaviours aren't allowed in RCU
> > > read-side critical sections. Fix this by using GFP_NOWAIT instead.
> >
> > This would be another way of fixing the problem that is slightly less
> > complex than my initial proposal, but does allow for using GFP_KERNEL
> > for fewer failures:
> >
> > @@ -1366,11 +1366,10 @@ static ssize_t xps_queue_show(struct net_device *dev, unsigned int index,
> > {
> > struct xps_dev_maps *dev_maps;
> > unsigned long *mask;
> > - unsigned int nr_ids;
> > + unsigned int nr_ids, new_nr_ids;
> > int j, len;
> >
> > - rcu_read_lock();
> > - dev_maps = rcu_dereference(dev->xps_maps[type]);
> > + dev_maps = READ_ONCE(dev->xps_maps[type]);
>
> Couldn't dev_maps be freed between here and the read of dev_maps->nr_ids
> as we're not in an RCU read-side critical section?
* The first read of dev_maps->nr_ids, happening before rcu_read_lock,
not the one shown below.
> > /* Default to nr_cpu_ids/dev->num_rx_queues and do not just return 0
> > * when dev_maps hasn't been allocated yet, to be backward compatible.
> > @@ -1379,10 +1378,18 @@ static ssize_t xps_queue_show(struct net_device *dev, unsigned int index,
> > (type == XPS_CPUS ? nr_cpu_ids : dev->num_rx_queues);
> >
> > mask = bitmap_zalloc(nr_ids, GFP_KERNEL);
> > - if (!mask) {
> > - rcu_read_unlock();
> > + if (!mask)
> > return -ENOMEM;
> > - }
> > +
> > + rcu_read_lock();
> > + dev_maps = rcu_dereference(dev->xps_maps[type]);
> > + /* if nr_ids shrank in the meantime, do not overrun array.
> > + * if it increased, we just won't show the new ones
> > + */
> > + new_nr_ids = dev_maps ? dev_maps->nr_ids :
> > + (type == XPS_CPUS ? nr_cpu_ids : dev->num_rx_queues);
> > + if (new_nr_ids < nr_ids)
> > + nr_ids = new_nr_ids;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net-sysfs: remove possible sleep from an RCU read-side critical section
2021-03-22 17:44 ` Matthew Wilcox
@ 2021-03-22 17:49 ` Antoine Tenart
0 siblings, 0 replies; 7+ messages in thread
From: Antoine Tenart @ 2021-03-22 17:49 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: davem, kuba, alexander.duyck, netdev, kernel test robot
Quoting Matthew Wilcox (2021-03-22 18:44:21)
> On Mon, Mar 22, 2021 at 06:41:30PM +0100, Antoine Tenart wrote:
> > Quoting Matthew Wilcox (2021-03-22 17:54:39)
> > > - rcu_read_lock();
> > > - dev_maps = rcu_dereference(dev->xps_maps[type]);
> > > + dev_maps = READ_ONCE(dev->xps_maps[type]);
> >
> > Couldn't dev_maps be freed between here and the read of dev_maps->nr_ids
> > as we're not in an RCU read-side critical section?
>
> Oh, good point. Never mind, then.
>
> > My feeling is there is not much value in having a tricky allocation
> > logic for reads from xps_cpus and xps_rxqs. While we could come up with
> > something, returning -ENOMEM on memory pressure should be fine.
>
> That's fine. It's your code, and this is probably a small allocation
> anyway.
All right. Thanks for the suggestions anyway!
Antoine
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net-sysfs: remove possible sleep from an RCU read-side critical section
2021-03-22 15:43 [PATCH net-next] net-sysfs: remove possible sleep from an RCU read-side critical section Antoine Tenart
2021-03-22 16:54 ` Matthew Wilcox
@ 2021-03-22 20:40 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-03-22 20:40 UTC (permalink / raw)
To: Antoine Tenart; +Cc: davem, kuba, alexander.duyck, netdev, oliver.sang, willy
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Mon, 22 Mar 2021 16:43:29 +0100 you wrote:
> xps_queue_show is mostly made of an RCU read-side critical section and
> calls bitmap_zalloc with GFP_KERNEL in the middle of it. That is not
> allowed as this call may sleep and such behaviours aren't allowed in RCU
> read-side critical sections. Fix this by using GFP_NOWAIT instead.
>
> Fixes: 5478fcd0f483 ("net: embed nr_ids in the xps maps")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Antoine Tenart <atenart@kernel.org>
>
> [...]
Here is the summary with links:
- [net-next] net-sysfs: remove possible sleep from an RCU read-side critical section
https://git.kernel.org/netdev/net-next/c/7f08ec6e0426
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-22 20:40 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-22 15:43 [PATCH net-next] net-sysfs: remove possible sleep from an RCU read-side critical section Antoine Tenart
2021-03-22 16:54 ` Matthew Wilcox
2021-03-22 17:41 ` Antoine Tenart
2021-03-22 17:44 ` Matthew Wilcox
2021-03-22 17:49 ` Antoine Tenart
2021-03-22 17:45 ` Antoine Tenart
2021-03-22 20:40 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.