| Message ID | 20221018120111.1474581-1-gongruiqi1@huawei.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | selinux: use GFP_ATOMIC in convert_context() | expand |
On Tue, Oct 18, 2022 at 2:01 PM GONG, Ruiqi <gongruiqi1@huawei.com> wrote: > The following BUG_ON was triggered on a hardware environment: > > SELinux: Converting 162 SID table entries... > BUG: sleeping function called from invalid context at __might_sleep_rtos+0x60/0x74 0x0 > in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 5943, name: tar > CPU: 7 PID: 5943 Comm: tar Tainted: P O 5.10.0 #1 > Call trace: > dump_backtrace+0x0/0x1c8 > show_stack+0x18/0x28 > dump_stack+0xe8/0x15c > ___might_sleep_rtos+0x168/0x17c > __might_sleep_rtos+0x60/0x74 > __kmalloc_track_caller+0xa0/0x7dc > kstrdup+0x54/0xac > convert_context+0x48/0x2e4 > sidtab_context_to_sid+0x1c4/0x36c > security_context_to_sid_core+0x168/0x238 > security_context_to_sid_default+0x14/0x24 > inode_doinit_use_xattr+0x164/0x1e4 > inode_doinit_with_dentry+0x1c0/0x488 > selinux_d_instantiate+0x20/0x34 > security_d_instantiate+0x70/0xbc > d_splice_alias+0x4c/0x3c0 > ext4_lookup+0x1d8/0x200 [ext4] > __lookup_slow+0x12c/0x1e4 > walk_component+0x100/0x200 > path_lookupat+0x88/0x118 > filename_lookup+0x98/0x130 > user_path_at_empty+0x48/0x60 > vfs_statx+0x84/0x140 > vfs_fstatat+0x20/0x30 > __se_sys_newfstatat+0x30/0x74 > __arm64_sys_newfstatat+0x1c/0x2c > el0_svc_common.constprop.0+0x100/0x184 > do_el0_svc+0x1c/0x2c > el0_svc+0x20/0x34 > el0_sync_handler+0x80/0x17c > el0_sync+0x13c/0x140 > SELinux: Context system_u:object_r:pssp_rsyslog_log_t:s0:c0 is not valid (left unmapped). > > It was found that convert_context() (hooked by convert->func) might > sleep in a critial section of spin_lock_irqsave in > sidtab_context_to_sid(). Fix this problem by changing the memory > allocation in convert_context() from GFP_KERNEL to GFP_ATOMIC. Good catch! However, convert_context() (and sidtab_convert_params::func) has two callers: 1. sidtab_context_to_sid(), which requires GFP_ATOMIC, 2. sidtab_convert_tree()/sidtab_convert(), where GFP_KERNEL would be okay. So a more optimal fix would be to add a gfp_t argument to convert_context()/sidtab_convert_params::func and pass GFP_KERNEL/_ATOMIC as appropriate in the individual callers. > Reported-by: Tan Ninghao <tanninghao1@huawei.com> > Fixes: ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve performance") > Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com> > --- > security/selinux/ss/services.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index fe5fcf571c56..523876bb7df3 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -2036,7 +2036,7 @@ static int convert_context(struct context *oldc, struct context *newc, void *p) > args = p; > > if (oldc->str) { > - s = kstrdup(oldc->str, GFP_KERNEL); > + s = kstrdup(oldc->str, GFP_ATOMIC); > if (!s) > return -ENOMEM; > > -- > 2.25.1 >
On Tue, Oct 18, 2022 at 8:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Tue, Oct 18, 2022 at 2:01 PM GONG, Ruiqi <gongruiqi1@huawei.com> wrote: > > The following BUG_ON was triggered on a hardware environment: > > > > SELinux: Converting 162 SID table entries... > > BUG: sleeping function called from invalid context at __might_sleep_rtos+0x60/0x74 0x0 > > in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 5943, name: tar > > CPU: 7 PID: 5943 Comm: tar Tainted: P O 5.10.0 #1 > > Call trace: > > dump_backtrace+0x0/0x1c8 > > show_stack+0x18/0x28 > > dump_stack+0xe8/0x15c > > ___might_sleep_rtos+0x168/0x17c > > __might_sleep_rtos+0x60/0x74 > > __kmalloc_track_caller+0xa0/0x7dc > > kstrdup+0x54/0xac > > convert_context+0x48/0x2e4 > > sidtab_context_to_sid+0x1c4/0x36c > > security_context_to_sid_core+0x168/0x238 > > security_context_to_sid_default+0x14/0x24 > > inode_doinit_use_xattr+0x164/0x1e4 > > inode_doinit_with_dentry+0x1c0/0x488 > > selinux_d_instantiate+0x20/0x34 > > security_d_instantiate+0x70/0xbc > > d_splice_alias+0x4c/0x3c0 > > ext4_lookup+0x1d8/0x200 [ext4] > > __lookup_slow+0x12c/0x1e4 > > walk_component+0x100/0x200 > > path_lookupat+0x88/0x118 > > filename_lookup+0x98/0x130 > > user_path_at_empty+0x48/0x60 > > vfs_statx+0x84/0x140 > > vfs_fstatat+0x20/0x30 > > __se_sys_newfstatat+0x30/0x74 > > __arm64_sys_newfstatat+0x1c/0x2c > > el0_svc_common.constprop.0+0x100/0x184 > > do_el0_svc+0x1c/0x2c > > el0_svc+0x20/0x34 > > el0_sync_handler+0x80/0x17c > > el0_sync+0x13c/0x140 > > SELinux: Context system_u:object_r:pssp_rsyslog_log_t:s0:c0 is not valid (left unmapped). > > > > It was found that convert_context() (hooked by convert->func) might > > sleep in a critial section of spin_lock_irqsave in > > sidtab_context_to_sid(). Fix this problem by changing the memory > > allocation in convert_context() from GFP_KERNEL to GFP_ATOMIC. > > Good catch! However, convert_context() (and > sidtab_convert_params::func) has two callers: > 1. sidtab_context_to_sid(), which requires GFP_ATOMIC, > 2. sidtab_convert_tree()/sidtab_convert(), where GFP_KERNEL would be okay. > > So a more optimal fix would be to add a gfp_t argument to > convert_context()/sidtab_convert_params::func and pass > GFP_KERNEL/_ATOMIC as appropriate in the individual callers. Agreed. Please make the changes Ondrej is suggesting.
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index fe5fcf571c56..523876bb7df3 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2036,7 +2036,7 @@ static int convert_context(struct context *oldc, struct context *newc, void *p) args = p; if (oldc->str) { - s = kstrdup(oldc->str, GFP_KERNEL); + s = kstrdup(oldc->str, GFP_ATOMIC); if (!s) return -ENOMEM;
The following BUG_ON was triggered on a hardware environment: SELinux: Converting 162 SID table entries... BUG: sleeping function called from invalid context at __might_sleep_rtos+0x60/0x74 0x0 in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 5943, name: tar CPU: 7 PID: 5943 Comm: tar Tainted: P O 5.10.0 #1 Call trace: dump_backtrace+0x0/0x1c8 show_stack+0x18/0x28 dump_stack+0xe8/0x15c ___might_sleep_rtos+0x168/0x17c __might_sleep_rtos+0x60/0x74 __kmalloc_track_caller+0xa0/0x7dc kstrdup+0x54/0xac convert_context+0x48/0x2e4 sidtab_context_to_sid+0x1c4/0x36c security_context_to_sid_core+0x168/0x238 security_context_to_sid_default+0x14/0x24 inode_doinit_use_xattr+0x164/0x1e4 inode_doinit_with_dentry+0x1c0/0x488 selinux_d_instantiate+0x20/0x34 security_d_instantiate+0x70/0xbc d_splice_alias+0x4c/0x3c0 ext4_lookup+0x1d8/0x200 [ext4] __lookup_slow+0x12c/0x1e4 walk_component+0x100/0x200 path_lookupat+0x88/0x118 filename_lookup+0x98/0x130 user_path_at_empty+0x48/0x60 vfs_statx+0x84/0x140 vfs_fstatat+0x20/0x30 __se_sys_newfstatat+0x30/0x74 __arm64_sys_newfstatat+0x1c/0x2c el0_svc_common.constprop.0+0x100/0x184 do_el0_svc+0x1c/0x2c el0_svc+0x20/0x34 el0_sync_handler+0x80/0x17c el0_sync+0x13c/0x140 SELinux: Context system_u:object_r:pssp_rsyslog_log_t:s0:c0 is not valid (left unmapped). It was found that convert_context() (hooked by convert->func) might sleep in a critial section of spin_lock_irqsave in sidtab_context_to_sid(). Fix this problem by changing the memory allocation in convert_context() from GFP_KERNEL to GFP_ATOMIC. Reported-by: Tan Ninghao <tanninghao1@huawei.com> Fixes: ee1a84fdfeed ("selinux: overhaul sidtab to fix bug and improve performance") Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com> --- security/selinux/ss/services.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)