* [PATCH] net:qrtr: fix allocator flag of idr_alloc_u32() in qrtr_port_assign()
@ 2021-03-26 3:33 Du Cheng
2021-03-26 9:31 ` Greg Kroah-Hartman
0 siblings, 1 reply; 4+ messages in thread
From: Du Cheng @ 2021-03-26 3:33 UTC (permalink / raw
To: David S. Miller, Jakub Kicinski
Cc: netdev, Greg Kroah-Hartman, Shuah Khan, Du Cheng,
syzbot+3eec59e770685e3dc879
change the allocator flag of idr_alloc_u32 from GFP_ATOMIC to
GFP_KERNEL, as GFP_ATOMIC caused BUG: "using smp_processor_id() in
preemptible" as reported by syzkaller.
Reported-by: syzbot+3eec59e770685e3dc879@syzkaller.appspotmail.com
Signed-off-by: Du Cheng <ducheng2@gmail.com>
---
Hi David & Jakub,
Although this is a simple fix to make syzkaller happy, I feel that maybe a more
proper fix is to convert qrtr_ports from using IDR to radix_tree (which is in
fact xarray) ?
I found some previous work done in 2019 by Matthew Wilcox:
https://lore.kernel.org/netdev/20190820223259.22348-1-willy@infradead.org/t/#mcb60ad4c34e35a6183c7353c8a44ceedfcff297d
but that was not merged as of now. My wild guess is that it was probably
in conflicti with the conversion of radix_tree to xarray during 2020, and that
might cause the direct use of xarray in qrtr.c unfavorable.
Shall I proceed with converting qrtr_pors to use radix_tree (or just xarray)?
Regards,
Du Cheng
net/qrtr/qrtr.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index edb6ac17ceca..ee42e1e1d4d4 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -722,17 +722,17 @@ static int qrtr_port_assign(struct qrtr_sock *ipc, int *port)
mutex_lock(&qrtr_port_lock);
if (!*port) {
min_port = QRTR_MIN_EPH_SOCKET;
- rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, QRTR_MAX_EPH_SOCKET, GFP_ATOMIC);
+ rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, QRTR_MAX_EPH_SOCKET, GFP_KERNEL);
if (!rc)
*port = min_port;
} else if (*port < QRTR_MIN_EPH_SOCKET && !capable(CAP_NET_ADMIN)) {
rc = -EACCES;
} else if (*port == QRTR_PORT_CTRL) {
min_port = 0;
- rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, 0, GFP_ATOMIC);
+ rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, 0, GFP_KERNEL);
} else {
min_port = *port;
- rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, *port, GFP_ATOMIC);
+ rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, *port, GFP_KERNEL);
if (!rc)
*port = min_port;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] net:qrtr: fix allocator flag of idr_alloc_u32() in qrtr_port_assign()
2021-03-26 3:33 [PATCH] net:qrtr: fix allocator flag of idr_alloc_u32() in qrtr_port_assign() Du Cheng
@ 2021-03-26 9:31 ` Greg Kroah-Hartman
2021-03-27 1:44 ` Du Cheng
0 siblings, 1 reply; 4+ messages in thread
From: Greg Kroah-Hartman @ 2021-03-26 9:31 UTC (permalink / raw
To: Du Cheng
Cc: David S. Miller, Jakub Kicinski, netdev, Shuah Khan,
syzbot+3eec59e770685e3dc879
On Fri, Mar 26, 2021 at 11:33:45AM +0800, Du Cheng wrote:
> change the allocator flag of idr_alloc_u32 from GFP_ATOMIC to
> GFP_KERNEL, as GFP_ATOMIC caused BUG: "using smp_processor_id() in
> preemptible" as reported by syzkaller.
>
> Reported-by: syzbot+3eec59e770685e3dc879@syzkaller.appspotmail.com
> Signed-off-by: Du Cheng <ducheng2@gmail.com>
> ---
> Hi David & Jakub,
>
> Although this is a simple fix to make syzkaller happy, I feel that maybe a more
> proper fix is to convert qrtr_ports from using IDR to radix_tree (which is in
> fact xarray) ?
>
> I found some previous work done in 2019 by Matthew Wilcox:
> https://lore.kernel.org/netdev/20190820223259.22348-1-willy@infradead.org/t/#mcb60ad4c34e35a6183c7353c8a44ceedfcff297d
> but that was not merged as of now. My wild guess is that it was probably
> in conflicti with the conversion of radix_tree to xarray during 2020, and that
> might cause the direct use of xarray in qrtr.c unfavorable.
>
> Shall I proceed with converting qrtr_pors to use radix_tree (or just xarray)?
Try it and see. But how would that resolve this issue? Those other
structures would also need to allocate memory at this point in time and
you need to tell it if it can sleep or not.
> diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
> index edb6ac17ceca..ee42e1e1d4d4 100644
> --- a/net/qrtr/qrtr.c
> +++ b/net/qrtr/qrtr.c
> @@ -722,17 +722,17 @@ static int qrtr_port_assign(struct qrtr_sock *ipc, int *port)
> mutex_lock(&qrtr_port_lock);
> if (!*port) {
> min_port = QRTR_MIN_EPH_SOCKET;
> - rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, QRTR_MAX_EPH_SOCKET, GFP_ATOMIC);
> + rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, QRTR_MAX_EPH_SOCKET, GFP_KERNEL);
Are you sure that you can sleep in this code path?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] net:qrtr: fix allocator flag of idr_alloc_u32() in qrtr_port_assign()
2021-03-26 9:31 ` Greg Kroah-Hartman
@ 2021-03-27 1:44 ` Du Cheng
2021-03-27 11:46 ` Greg Kroah-Hartman
0 siblings, 1 reply; 4+ messages in thread
From: Du Cheng @ 2021-03-27 1:44 UTC (permalink / raw
To: Greg Kroah-Hartman
Cc: David S. Miller, Jakub Kicinski, netdev, Shuah Khan,
syzbot+3eec59e770685e3dc879
On Fri, Mar 26, 2021 at 10:31:57AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Mar 26, 2021 at 11:33:45AM +0800, Du Cheng wrote:
> > change the allocator flag of idr_alloc_u32 from GFP_ATOMIC to
> > GFP_KERNEL, as GFP_ATOMIC caused BUG: "using smp_processor_id() in
> > preemptible" as reported by syzkaller.
> >
> > Reported-by: syzbot+3eec59e770685e3dc879@syzkaller.appspotmail.com
> > Signed-off-by: Du Cheng <ducheng2@gmail.com>
> > ---
> > Hi David & Jakub,
> >
> > Although this is a simple fix to make syzkaller happy, I feel that maybe a more
> > proper fix is to convert qrtr_ports from using IDR to radix_tree (which is in
> > fact xarray) ?
> >
> > I found some previous work done in 2019 by Matthew Wilcox:
> > https://lore.kernel.org/netdev/20190820223259.22348-1-willy@infradead.org/t/#mcb60ad4c34e35a6183c7353c8a44ceedfcff297d
> > but that was not merged as of now. My wild guess is that it was probably
> > in conflicti with the conversion of radix_tree to xarray during 2020, and that
> > might cause the direct use of xarray in qrtr.c unfavorable.
> >
> > Shall I proceed with converting qrtr_pors to use radix_tree (or just xarray)?
Hi Greg,
After more scrutiny, this is entirely unnecessary, as the idr structure is
implemented as a radix_tree, which is, you guess it, xarray :)
So I looked more closely, and this time I found the culprit of the crash. It was
due to a unprotected per_cpu access:
```
rtp = this_cpu_ptr(&radix_tree_preloads);
if (rtp->nr) {
ret = rtp->nodes;
rtp->nodes = ret->parent;
rtp->nr--;
}
```
inside
-> radix_tree_node_alloc()
-> idr_get_free()
idr_alloc_u32()
I tried to wrap the idr_alloc_u32() with disable_preemption() and
enable_preemption(), and it passed my local and syzbot test.
More digging reveals that idr routines provide such utilities:
idr_preload() and idr_preload_end(). They do the exact thing but with additional
radix_tree bookkeeping. Hence I think this should be favorable than allowing
the allocation to sleep. The syzbot-passed patch is here:
https://syzkaller.appspot.com/text?tag=Patch&x=14cf5a26d00000
If it looks good to you, I will send the above patch as V2.
>
> Try it and see. But how would that resolve this issue? Those other
> structures would also need to allocate memory at this point in time and
> you need to tell it if it can sleep or not.
>
> > diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
> > index edb6ac17ceca..ee42e1e1d4d4 100644
> > --- a/net/qrtr/qrtr.c
> > +++ b/net/qrtr/qrtr.c
> > @@ -722,17 +722,17 @@ static int qrtr_port_assign(struct qrtr_sock *ipc, int *port)
> > mutex_lock(&qrtr_port_lock);
> > if (!*port) {
> > min_port = QRTR_MIN_EPH_SOCKET;
> > - rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, QRTR_MAX_EPH_SOCKET, GFP_ATOMIC);
> > + rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, QRTR_MAX_EPH_SOCKET, GFP_KERNEL);
>
> Are you sure that you can sleep in this code path?
There are only 2 other places there the mutex is held, and they seem to be safe,
but I can't show that comprehensively.
If I *were* to go with sleeping in idr_alloc_u32, does lockdep a silverbullet to
prove lock safty?
>
> thanks,
>
> greg k-h
Regards,
Du Cheng
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] net:qrtr: fix allocator flag of idr_alloc_u32() in qrtr_port_assign()
2021-03-27 1:44 ` Du Cheng
@ 2021-03-27 11:46 ` Greg Kroah-Hartman
0 siblings, 0 replies; 4+ messages in thread
From: Greg Kroah-Hartman @ 2021-03-27 11:46 UTC (permalink / raw
To: Du Cheng
Cc: David S. Miller, Jakub Kicinski, netdev, Shuah Khan,
syzbot+3eec59e770685e3dc879
On Sat, Mar 27, 2021 at 09:44:37AM +0800, Du Cheng wrote:
> On Fri, Mar 26, 2021 at 10:31:57AM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Mar 26, 2021 at 11:33:45AM +0800, Du Cheng wrote:
> > > change the allocator flag of idr_alloc_u32 from GFP_ATOMIC to
> > > GFP_KERNEL, as GFP_ATOMIC caused BUG: "using smp_processor_id() in
> > > preemptible" as reported by syzkaller.
> > >
> > > Reported-by: syzbot+3eec59e770685e3dc879@syzkaller.appspotmail.com
> > > Signed-off-by: Du Cheng <ducheng2@gmail.com>
> > > ---
> > > Hi David & Jakub,
> > >
> > > Although this is a simple fix to make syzkaller happy, I feel that maybe a more
> > > proper fix is to convert qrtr_ports from using IDR to radix_tree (which is in
> > > fact xarray) ?
> > >
> > > I found some previous work done in 2019 by Matthew Wilcox:
> > > https://lore.kernel.org/netdev/20190820223259.22348-1-willy@infradead.org/t/#mcb60ad4c34e35a6183c7353c8a44ceedfcff297d
> > > but that was not merged as of now. My wild guess is that it was probably
> > > in conflicti with the conversion of radix_tree to xarray during 2020, and that
> > > might cause the direct use of xarray in qrtr.c unfavorable.
> > >
> > > Shall I proceed with converting qrtr_pors to use radix_tree (or just xarray)?
>
> Hi Greg,
>
> After more scrutiny, this is entirely unnecessary, as the idr structure is
> implemented as a radix_tree, which is, you guess it, xarray :)
>
> So I looked more closely, and this time I found the culprit of the crash. It was
> due to a unprotected per_cpu access:
> ```
> rtp = this_cpu_ptr(&radix_tree_preloads);
> if (rtp->nr) {
> ret = rtp->nodes;
> rtp->nodes = ret->parent;
> rtp->nr--;
> }
> ```
> inside
> -> radix_tree_node_alloc()
> -> idr_get_free()
> idr_alloc_u32()
>
> I tried to wrap the idr_alloc_u32() with disable_preemption() and
> enable_preemption(), and it passed my local and syzbot test.
>
> More digging reveals that idr routines provide such utilities:
> idr_preload() and idr_preload_end(). They do the exact thing but with additional
> radix_tree bookkeeping. Hence I think this should be favorable than allowing
> the allocation to sleep. The syzbot-passed patch is here:
> https://syzkaller.appspot.com/text?tag=Patch&x=14cf5a26d00000
>
> If it looks good to you, I will send the above patch as V2.
If that resolves the issue, then that's fine with me.
> > Try it and see. But how would that resolve this issue? Those other
> > structures would also need to allocate memory at this point in time and
> > you need to tell it if it can sleep or not.
> >
> > > diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
> > > index edb6ac17ceca..ee42e1e1d4d4 100644
> > > --- a/net/qrtr/qrtr.c
> > > +++ b/net/qrtr/qrtr.c
> > > @@ -722,17 +722,17 @@ static int qrtr_port_assign(struct qrtr_sock *ipc, int *port)
> > > mutex_lock(&qrtr_port_lock);
> > > if (!*port) {
> > > min_port = QRTR_MIN_EPH_SOCKET;
> > > - rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, QRTR_MAX_EPH_SOCKET, GFP_ATOMIC);
> > > + rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, QRTR_MAX_EPH_SOCKET, GFP_KERNEL);
> >
> > Are you sure that you can sleep in this code path?
> There are only 2 other places there the mutex is held, and they seem to be safe,
> but I can't show that comprehensively.
> If I *were* to go with sleeping in idr_alloc_u32, does lockdep a silverbullet to
> prove lock safty?
I do not think lockdep does not test sleeping stuff, it just checks the
order in which locks are held.
You should be able to trace back the code paths here to ensure that
these functions are called in safe context or not, that might be worth
the effort here to make this fix simpler.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-27 11:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-26 3:33 [PATCH] net:qrtr: fix allocator flag of idr_alloc_u32() in qrtr_port_assign() Du Cheng
2021-03-26 9:31 ` Greg Kroah-Hartman
2021-03-27 1:44 ` Du Cheng
2021-03-27 11:46 ` Greg Kroah-Hartman
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.