* Preemptible idr_alloc() in QRTR code @ 2021-01-26 10:47 Mark Rutland 2021-01-26 14:58 ` Matthew Wilcox 0 siblings, 1 reply; 6+ messages in thread From: Mark Rutland @ 2021-01-26 10:47 UTC (permalink / raw) To: linux-kernel, netdev; +Cc: Matthew Wilcox, Courtney Cavin, Bjorn Andersson Hi, When fuzzing arm64 with Syzkaller, I'm seeing some splats where this_cpu_ptr() is used in the bowels of idr_alloc(), by way of radix_tree_node_alloc(), in a preemptible context: | BUG: using smp_processor_id() in preemptible [00000000] code: syz-executor.1/32582 | caller is debug_smp_processor_id+0x24/0x30 | CPU: 3 PID: 32582 Comm: syz-executor.1 Not tainted 5.11.0-rc4-next-20210125-00001-gf57e7edf910d #3 | Hardware name: linux,dummy-virt (DT) | Call trace: | dump_backtrace+0x0/0x4a8 | show_stack+0x34/0x88 | dump_stack+0x1d4/0x2a0 | check_preemption_disabled+0x1b8/0x210 | debug_smp_processor_id+0x24/0x30 | radix_tree_node_alloc.constprop.17+0x26c/0x3d0 | radix_tree_extend+0x200/0x420 | idr_get_free+0x63c/0xa38 | idr_alloc_u32+0x164/0x2a0 | __qrtr_bind.isra.8+0x350/0x658 | qrtr_bind+0x18c/0x218 | __sys_bind+0x1fc/0x238 | __arm64_sys_bind+0x78/0xb0 | el0_svc_common+0x1ac/0x4c8 | do_el0_svc+0xfc/0x150 | el0_svc+0x24/0x38 | el0_sync_handler+0x134/0x180 | el0_sync+0x154/0x180 It's not clear to me whether this is a bug in the caller or a bug the implementation of idr_alloc(). The kerneldoc for idr_alloc() mentions that callers must provide their own locking (and in this case a mutex is used), but doesn't mention that preemption must be disabled. Is this an intentional requirement that's simply missing from the documentation and requires a change to the QRTR code, or is this something to fix within the bowels of idr_alloc() and its callees? Thanks, Mark. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Preemptible idr_alloc() in QRTR code 2021-01-26 10:47 Preemptible idr_alloc() in QRTR code Mark Rutland @ 2021-01-26 14:58 ` Matthew Wilcox 2021-01-26 16:21 ` Mark Rutland 0 siblings, 1 reply; 6+ messages in thread From: Matthew Wilcox @ 2021-01-26 14:58 UTC (permalink / raw) To: Mark Rutland; +Cc: linux-kernel, netdev, Courtney Cavin, Bjorn Andersson On Tue, Jan 26, 2021 at 10:47:34AM +0000, Mark Rutland wrote: > Hi, > > When fuzzing arm64 with Syzkaller, I'm seeing some splats where > this_cpu_ptr() is used in the bowels of idr_alloc(), by way of > radix_tree_node_alloc(), in a preemptible context: I sent a patch to fix this last June. The maintainer seems to be under the impression that I care an awful lot more about their code than I do. https://lore.kernel.org/netdev/20200605120037.17427-1-willy@infradead.org/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Preemptible idr_alloc() in QRTR code 2021-01-26 14:58 ` Matthew Wilcox @ 2021-01-26 16:21 ` Mark Rutland 2021-01-26 17:00 ` Bjorn Andersson 0 siblings, 1 reply; 6+ messages in thread From: Mark Rutland @ 2021-01-26 16:21 UTC (permalink / raw) To: Matthew Wilcox, Bjorn Andersson; +Cc: linux-kernel, netdev, Courtney Cavin On Tue, Jan 26, 2021 at 02:58:33PM +0000, Matthew Wilcox wrote: > On Tue, Jan 26, 2021 at 10:47:34AM +0000, Mark Rutland wrote: > > Hi, > > > > When fuzzing arm64 with Syzkaller, I'm seeing some splats where > > this_cpu_ptr() is used in the bowels of idr_alloc(), by way of > > radix_tree_node_alloc(), in a preemptible context: > > I sent a patch to fix this last June. The maintainer seems to be > under the impression that I care an awful lot more about their > code than I do. > > https://lore.kernel.org/netdev/20200605120037.17427-1-willy@infradead.org/ Ah; I hadn't spotted the (glaringly obvious) GFP_ATOMIC abuse, thanks for the pointer, and sorry for the noise. It looks like Eric was after a fix that trivially backported to v4.7 (and hence couldn't rely on xarray) but instead it just got left broken for months. :/ Bjorn, is this something you care about? You seem to have the most commits to the file, and otherwise the official maintainer is Dave Miller per get_maintainer.pl. It is very tempting to make the config option depend on BROKEN... Thanks, Mark. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Preemptible idr_alloc() in QRTR code 2021-01-26 16:21 ` Mark Rutland @ 2021-01-26 17:00 ` Bjorn Andersson 2021-01-26 18:36 ` Mark Rutland 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Andersson @ 2021-01-26 17:00 UTC (permalink / raw) To: Mark Rutland; +Cc: Matthew Wilcox, linux-kernel, netdev, Courtney Cavin On Tue 26 Jan 10:21 CST 2021, Mark Rutland wrote: > On Tue, Jan 26, 2021 at 02:58:33PM +0000, Matthew Wilcox wrote: > > On Tue, Jan 26, 2021 at 10:47:34AM +0000, Mark Rutland wrote: > > > Hi, > > > > > > When fuzzing arm64 with Syzkaller, I'm seeing some splats where > > > this_cpu_ptr() is used in the bowels of idr_alloc(), by way of > > > radix_tree_node_alloc(), in a preemptible context: > > > > I sent a patch to fix this last June. The maintainer seems to be > > under the impression that I care an awful lot more about their > > code than I do. > > > > https://lore.kernel.org/netdev/20200605120037.17427-1-willy@infradead.org/ > > Ah; I hadn't spotted the (glaringly obvious) GFP_ATOMIC abuse, thanks > for the pointer, and sorry for the noise. > I'm afraid this isn't as obvious to me as it is to you. Are you saying that one must not use GFP_ATOMIC in non-atomic contexts? That said, glancing at the code I'm puzzled to why it would use GFP_ATOMIC. > It looks like Eric was after a fix that trivially backported to v4.7 > (and hence couldn't rely on xarray) but instead it just got left broken > for months. :/ > > Bjorn, is this something you care about? You seem to have the most > commits to the file, and otherwise the official maintainer is Dave > Miller per get_maintainer.pl. > I certainly care about qrtr working and remember glancing at Matthew's patch, but seems like I never found time to properly review it. > It is very tempting to make the config option depend on BROKEN... > I hear you and that would be bad, so I'll make sure to take a proper look at this and Matthew's patch. Thanks, Bjorn ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Preemptible idr_alloc() in QRTR code 2021-01-26 17:00 ` Bjorn Andersson @ 2021-01-26 18:36 ` Mark Rutland 2021-01-27 0:41 ` Matthew Wilcox 0 siblings, 1 reply; 6+ messages in thread From: Mark Rutland @ 2021-01-26 18:36 UTC (permalink / raw) To: Bjorn Andersson; +Cc: Matthew Wilcox, linux-kernel, netdev, Courtney Cavin On Tue, Jan 26, 2021 at 11:00:05AM -0600, Bjorn Andersson wrote: > On Tue 26 Jan 10:21 CST 2021, Mark Rutland wrote: > > > On Tue, Jan 26, 2021 at 02:58:33PM +0000, Matthew Wilcox wrote: > > > On Tue, Jan 26, 2021 at 10:47:34AM +0000, Mark Rutland wrote: > > > > Hi, > > > > > > > > When fuzzing arm64 with Syzkaller, I'm seeing some splats where > > > > this_cpu_ptr() is used in the bowels of idr_alloc(), by way of > > > > radix_tree_node_alloc(), in a preemptible context: > > > > > > I sent a patch to fix this last June. The maintainer seems to be > > > under the impression that I care an awful lot more about their > > > code than I do. > > > > > > https://lore.kernel.org/netdev/20200605120037.17427-1-willy@infradead.org/ > > > > Ah; I hadn't spotted the (glaringly obvious) GFP_ATOMIC abuse, thanks > > for the pointer, and sorry for the noise. > > > > I'm afraid this isn't as obvious to me as it is to you. Are you saying > that one must not use GFP_ATOMIC in non-atomic contexts? > > That said, glancing at the code I'm puzzled to why it would use > GFP_ATOMIC. I'm also not entirely sure about the legitimacy of GFP_ATOMIC outside of atomic contexts -- I couldn't spot any documentation saying that wasn't legitimate, but Matthew's commit message implies so, and it sticks out as odd. > > It looks like Eric was after a fix that trivially backported to v4.7 > > (and hence couldn't rely on xarray) but instead it just got left broken > > for months. :/ > > > > Bjorn, is this something you care about? You seem to have the most > > commits to the file, and otherwise the official maintainer is Dave > > Miller per get_maintainer.pl. > > I certainly care about qrtr working and remember glancing at Matthew's > patch, but seems like I never found time to properly review it. > > > It is very tempting to make the config option depend on BROKEN... > > I hear you and that would be bad, so I'll make sure to take a proper > look at this and Matthew's patch. Thanks! I'm happy to try/test patches if that's any help. My main concern here is that this can be triggered on arbitrary platforms so long as the driver is built in (e.g. my Syzkaller instance is hitting this within a VM). Thanks, Mark. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Preemptible idr_alloc() in QRTR code 2021-01-26 18:36 ` Mark Rutland @ 2021-01-27 0:41 ` Matthew Wilcox 0 siblings, 0 replies; 6+ messages in thread From: Matthew Wilcox @ 2021-01-27 0:41 UTC (permalink / raw) To: Mark Rutland; +Cc: Bjorn Andersson, linux-kernel, netdev, Courtney Cavin On Tue, Jan 26, 2021 at 06:36:02PM +0000, Mark Rutland wrote: > On Tue, Jan 26, 2021 at 11:00:05AM -0600, Bjorn Andersson wrote: > > On Tue 26 Jan 10:21 CST 2021, Mark Rutland wrote: > > > > > On Tue, Jan 26, 2021 at 02:58:33PM +0000, Matthew Wilcox wrote: > > > > On Tue, Jan 26, 2021 at 10:47:34AM +0000, Mark Rutland wrote: > > > > > Hi, > > > > > > > > > > When fuzzing arm64 with Syzkaller, I'm seeing some splats where > > > > > this_cpu_ptr() is used in the bowels of idr_alloc(), by way of > > > > > radix_tree_node_alloc(), in a preemptible context: > > > > > > > > I sent a patch to fix this last June. The maintainer seems to be > > > > under the impression that I care an awful lot more about their > > > > code than I do. > > > > > > > > https://lore.kernel.org/netdev/20200605120037.17427-1-willy@infradead.org/ > > > > > > Ah; I hadn't spotted the (glaringly obvious) GFP_ATOMIC abuse, thanks > > > for the pointer, and sorry for the noise. > > > > > > > I'm afraid this isn't as obvious to me as it is to you. Are you saying > > that one must not use GFP_ATOMIC in non-atomic contexts? > > > > That said, glancing at the code I'm puzzled to why it would use > > GFP_ATOMIC. > > I'm also not entirely sure about the legitimacy of GFP_ATOMIC outside of > atomic contexts -- I couldn't spot any documentation saying that wasn't > legitimate, but Matthew's commit message implies so, and it sticks out > as odd. It's actually an assumption in the radix tree code. If you say you can't be preempted by saying GFP_ATOMIC, it takes you at your word and does some things which cannot be done in preemptable context. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-27 7:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-26 10:47 Preemptible idr_alloc() in QRTR code Mark Rutland 2021-01-26 14:58 ` Matthew Wilcox 2021-01-26 16:21 ` Mark Rutland 2021-01-26 17:00 ` Bjorn Andersson 2021-01-26 18:36 ` Mark Rutland 2021-01-27 0:41 ` Matthew Wilcox
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).