Re: commit_creds oops
Dave Jones writes: > On Thu, Feb 28, 2013 at 04:25:40PM -0800, Eric W. Biederman wrote: > > > > [ 89.639850] RIP: 0010:[] [] > commit_creds+0x250/0x2f0 > > > [ 89.658399] Call Trace: > > > [ 89.658822] [] > key_change_session_keyring+0xfb/0x140 > > > [ 89.659845] [] task_work_run+0xa5/0xd0 > > > [ 89.660698] [] do_notify_resume+0x71/0xb0 > > > [ 89.661581] [] int_signal+0x12/0x17 > > > > > > Appears to be.. > > > > > > if ((set_ns == subset_ns->parent) && > > > 850: 48 8b 8a c8 00 00 00mov0xc8(%rdx),%rcx > > > > > > from the inlined cred_cap_issubset > > > > Interesting. That line is protected with the check subset_ns != > > _user_ns so subset_ns->parent must be valid or subset_ns is not > > a proper user namespace. > > > > Ugh. I think I see what is going on and it is just silly. > > > > It looks like by historical accident we have been reading trying to set > > new->user_ns from new->user_ns. Which is totally silly as new->user_ns > > is NULL (as is every other field in new except session_keyring at that > > point). > > > > It looks like it is safe to sleep in key_change_session_keyring so why > > we just don't use prepare_creds there like everywhere else is beyond > > me. > > > > The intent is clearly to copy all of the fields from old to new so what > > we should be doing is is copying old->user_ns into new->user_ns. > > > > Dave can you verify that this patch fixes the oops? > > Looks like it. Haven't hit the same thing since applying your patch. > > I noticed though that get_user_ns bumps a refcount. Is this what we > want if we're just copying ? Yes. commit_creds(new) winds up finding old on the current process and calling put_cred(old). put_cred when the count drops to zero winds up calling put_cred_rcu which calls put_user_ns(old->user_ns); For the same reason we need an extra count on the user namespace new so that when it eventually is put and put_user_ns(new->user_ns) is called we don't have a negative count. Which is a long of way of saying yes we are adding another reference and we need to increase the reference count. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: commit_creds oops
On Thu, Feb 28, 2013 at 04:25:40PM -0800, Eric W. Biederman wrote: > > [ 89.639850] RIP: 0010:[] [] > > commit_creds+0x250/0x2f0 > > [ 89.658399] Call Trace: > > [ 89.658822] [] key_change_session_keyring+0xfb/0x140 > > [ 89.659845] [] task_work_run+0xa5/0xd0 > > [ 89.660698] [] do_notify_resume+0x71/0xb0 > > [ 89.661581] [] int_signal+0x12/0x17 > > > > Appears to be.. > > > > if ((set_ns == subset_ns->parent) && > > 850: 48 8b 8a c8 00 00 00mov0xc8(%rdx),%rcx > > > > from the inlined cred_cap_issubset > > Interesting. That line is protected with the check subset_ns != > _user_ns so subset_ns->parent must be valid or subset_ns is not > a proper user namespace. > > Ugh. I think I see what is going on and it is just silly. > > It looks like by historical accident we have been reading trying to set > new->user_ns from new->user_ns. Which is totally silly as new->user_ns > is NULL (as is every other field in new except session_keyring at that > point). > > It looks like it is safe to sleep in key_change_session_keyring so why > we just don't use prepare_creds there like everywhere else is beyond > me. > > The intent is clearly to copy all of the fields from old to new so what > we should be doing is is copying old->user_ns into new->user_ns. > > Dave can you verify that this patch fixes the oops? Looks like it. Haven't hit the same thing since applying your patch. I noticed though that get_user_ns bumps a refcount. Is this what we want if we're just copying ? Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: commit_creds oops
Quoting Eric W. Biederman (ebied...@xmission.com): > Dave Jones writes: > > > Just hit this on Linus' current tree. > > > > [ 89.621770] BUG: unable to handle kernel NULL pointer dereference at > > 00c8 > > [ 89.623111] IP: [] commit_creds+0x250/0x2f0 > > [ 89.624062] PGD 122bfd067 PUD 122bfe067 PMD 0 > > [ 89.624901] Oops: [#1] PREEMPT SMP > > [ 89.625678] Modules linked in: caif_socket caif netrom bridge hidp 8021q > > garp stp mrp rose llc2 af_rxrpc phonet af_key binfmt_misc bnep l2tp_ppp > > can_bcm l2tp_core pppoe pppox can_raw scsi_transport_iscsi ppp_generic slhc > > nfnetlink can ipt_ULOG ax25 decnet irda nfc rds x25 crc_ccitt appletalk atm > > ipx p8023 psnap p8022 llc lockd sunrpc ip6t_REJECT nf_conntrack_ipv6 > > nf_defrag_ipv6 xt_conntrack nf_conntrack ip6table_filter ip6_tables btusb > > bluetooth snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_pcm > > vhost_net snd_page_alloc snd_timer tun macvtap usb_debug snd rfkill > > microcode macvlan edac_core pcspkr serio_raw kvm_amd soundcore kvm r8169 mii > > [ 89.637846] CPU 2 > > [ 89.638175] Pid: 782, comm: trinity-main Not tainted 3.8.0+ #63 Gigabyte > > Technology Co., Ltd. GA-MA78GM-S2H/GA-MA78GM-S2H > > [ 89.639850] RIP: 0010:[] [] > > commit_creds+0x250/0x2f0 > > [ 89.641161] RSP: 0018:880115657eb8 EFLAGS: 00010207 > > [ 89.641984] RAX: 03e8 RBX: 88012688b000 RCX: > > > > [ 89.643069] RDX: RSI: 81c32960 RDI: > > 880105839600 > > [ 89.644167] RBP: 880115657ed8 R08: R09: > > > > [ 89.645254] R10: 0001 R11: 0246 R12: > > 880105839600 > > [ 89.646340] R13: 88011beea490 R14: 88011beea490 R15: > > > > [ 89.647431] FS: 7f3ac063b740() GS:88012b20() > > knlGS: > > [ 89.648660] CS: 0010 DS: ES: CR0: 8005003b > > [ 89.649548] CR2: 00c8 CR3: 000122bfc000 CR4: > > 07e0 > > [ 89.650635] DR0: DR1: DR2: > > > > [ 89.651723] DR3: DR6: 0ff0 DR7: > > 0400 > > [ 89.652812] Process trinity-main (pid: 782, threadinfo 880115656000, > > task 88011beea490) > > [ 89.654128] Stack: > > [ 89.654433] 8801058396a0 880105839600 > > 88011beeaa78 > > [ 89.655769] 880115657ef8 812c7d9b 82079be0 > > > > [ 89.657073] 880115657f28 8106c665 0002 > > 880115657f58 > > [ 89.658399] Call Trace: > > [ 89.658822] [] key_change_session_keyring+0xfb/0x140 > > [ 89.659845] [] task_work_run+0xa5/0xd0 > > [ 89.660698] [] do_notify_resume+0x71/0xb0 > > [ 89.661581] [] int_signal+0x12/0x17 > > [ 89.662385] Code: 24 90 00 00 00 48 8b b3 90 00 00 00 49 8b 4c 24 40 48 > > 39 f2 75 08 e9 83 00 00 00 48 89 ca 48 81 fa 60 29 c3 81 0f 84 41 fe ff ff > > <48> 8b 8a c8 00 00 00 48 39 ce 75 e4 3b 82 d0 00 00 00 0f 84 4b > > [ 89.667778] RIP [] commit_creds+0x250/0x2f0 > > [ 89.668733] RSP > > [ 89.669301] CR2: 00c8 > > > > My fastest trinity induced oops yet! > > > > > > Appears to be.. > > > > if ((set_ns == subset_ns->parent) && > > 850: 48 8b 8a c8 00 00 00mov0xc8(%rdx),%rcx > > > > from the inlined cred_cap_issubset > > Interesting. That line is protected with the check subset_ns != > _user_ns so subset_ns->parent must be valid or subset_ns is not > a proper user namespace. > > Ugh. I think I see what is going on and it is just silly. > > It looks like by historical accident we have been reading trying to set > new->user_ns from new->user_ns. Which is totally silly as new->user_ns > is NULL (as is every other field in new except session_keyring at that > point). > > It looks like it is safe to sleep in key_change_session_keyring so why > we just don't use prepare_creds there like everywhere else is beyond > me. > > The intent is clearly to copy all of the fields from old to new so what > we should be doing is is copying old->user_ns into new->user_ns. > > Dave can you verify that this patch fixes the oops? > > Signed-off-by: "Eric W. Biederman" Jinkeys - that should have stood out like a sore thumb. Acked-by: Serge Hallyn > --- > diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c > index 58dfe08..a571fad 100644 > --- a/security/keys/process_keys.c > +++ b/security/keys/process_keys.c > @@ -839,7 +839,7 @@ void key_change_session_keyring(struct callback_head > *twork) > new-> sgid = old-> sgid; > new->fsgid = old->fsgid; > new->user = get_uid(old->user); > - new->user_ns= get_user_ns(new->user_ns); > + new->user_ns= get_user_ns(old->user_ns); > new->group_info =
Re: commit_creds oops
Dave Jones writes: > Just hit this on Linus' current tree. > > [ 89.621770] BUG: unable to handle kernel NULL pointer dereference at > 00c8 > [ 89.623111] IP: [] commit_creds+0x250/0x2f0 > [ 89.624062] PGD 122bfd067 PUD 122bfe067 PMD 0 > [ 89.624901] Oops: [#1] PREEMPT SMP > [ 89.625678] Modules linked in: caif_socket caif netrom bridge hidp 8021q > garp stp mrp rose llc2 af_rxrpc phonet af_key binfmt_misc bnep l2tp_ppp > can_bcm l2tp_core pppoe pppox can_raw scsi_transport_iscsi ppp_generic slhc > nfnetlink can ipt_ULOG ax25 decnet irda nfc rds x25 crc_ccitt appletalk atm > ipx p8023 psnap p8022 llc lockd sunrpc ip6t_REJECT nf_conntrack_ipv6 > nf_defrag_ipv6 xt_conntrack nf_conntrack ip6table_filter ip6_tables btusb > bluetooth snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_pcm vhost_net > snd_page_alloc snd_timer tun macvtap usb_debug snd rfkill microcode macvlan > edac_core pcspkr serio_raw kvm_amd soundcore kvm r8169 mii > [ 89.637846] CPU 2 > [ 89.638175] Pid: 782, comm: trinity-main Not tainted 3.8.0+ #63 Gigabyte > Technology Co., Ltd. GA-MA78GM-S2H/GA-MA78GM-S2H > [ 89.639850] RIP: 0010:[] [] > commit_creds+0x250/0x2f0 > [ 89.641161] RSP: 0018:880115657eb8 EFLAGS: 00010207 > [ 89.641984] RAX: 03e8 RBX: 88012688b000 RCX: > > [ 89.643069] RDX: RSI: 81c32960 RDI: > 880105839600 > [ 89.644167] RBP: 880115657ed8 R08: R09: > > [ 89.645254] R10: 0001 R11: 0246 R12: > 880105839600 > [ 89.646340] R13: 88011beea490 R14: 88011beea490 R15: > > [ 89.647431] FS: 7f3ac063b740() GS:88012b20() > knlGS: > [ 89.648660] CS: 0010 DS: ES: CR0: 8005003b > [ 89.649548] CR2: 00c8 CR3: 000122bfc000 CR4: > 07e0 > [ 89.650635] DR0: DR1: DR2: > > [ 89.651723] DR3: DR6: 0ff0 DR7: > 0400 > [ 89.652812] Process trinity-main (pid: 782, threadinfo 880115656000, > task 88011beea490) > [ 89.654128] Stack: > [ 89.654433] 8801058396a0 880105839600 > 88011beeaa78 > [ 89.655769] 880115657ef8 812c7d9b 82079be0 > > [ 89.657073] 880115657f28 8106c665 0002 > 880115657f58 > [ 89.658399] Call Trace: > [ 89.658822] [] key_change_session_keyring+0xfb/0x140 > [ 89.659845] [] task_work_run+0xa5/0xd0 > [ 89.660698] [] do_notify_resume+0x71/0xb0 > [ 89.661581] [] int_signal+0x12/0x17 > [ 89.662385] Code: 24 90 00 00 00 48 8b b3 90 00 00 00 49 8b 4c 24 40 48 39 > f2 75 08 e9 83 00 00 00 48 89 ca 48 81 fa 60 29 c3 81 0f 84 41 fe ff ff <48> > 8b 8a c8 00 00 00 48 39 ce 75 e4 3b 82 d0 00 00 00 0f 84 4b > [ 89.667778] RIP [] commit_creds+0x250/0x2f0 > [ 89.668733] RSP > [ 89.669301] CR2: 00c8 > > My fastest trinity induced oops yet! > > > Appears to be.. > > if ((set_ns == subset_ns->parent) && > 850: 48 8b 8a c8 00 00 00mov0xc8(%rdx),%rcx > > from the inlined cred_cap_issubset Interesting. That line is protected with the check subset_ns != _user_ns so subset_ns->parent must be valid or subset_ns is not a proper user namespace. Ugh. I think I see what is going on and it is just silly. It looks like by historical accident we have been reading trying to set new->user_ns from new->user_ns. Which is totally silly as new->user_ns is NULL (as is every other field in new except session_keyring at that point). It looks like it is safe to sleep in key_change_session_keyring so why we just don't use prepare_creds there like everywhere else is beyond me. The intent is clearly to copy all of the fields from old to new so what we should be doing is is copying old->user_ns into new->user_ns. Dave can you verify that this patch fixes the oops? Signed-off-by: "Eric W. Biederman" --- diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c index 58dfe08..a571fad 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -839,7 +839,7 @@ void key_change_session_keyring(struct callback_head *twork) new-> sgid = old-> sgid; new->fsgid = old->fsgid; new->user = get_uid(old->user); - new->user_ns= get_user_ns(new->user_ns); + new->user_ns= get_user_ns(old->user_ns); new->group_info = get_group_info(old->group_info); new->securebits = old->securebits; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
commit_creds oops
Just hit this on Linus' current tree. [ 89.621770] BUG: unable to handle kernel NULL pointer dereference at 00c8 [ 89.623111] IP: [] commit_creds+0x250/0x2f0 [ 89.624062] PGD 122bfd067 PUD 122bfe067 PMD 0 [ 89.624901] Oops: [#1] PREEMPT SMP [ 89.625678] Modules linked in: caif_socket caif netrom bridge hidp 8021q garp stp mrp rose llc2 af_rxrpc phonet af_key binfmt_misc bnep l2tp_ppp can_bcm l2tp_core pppoe pppox can_raw scsi_transport_iscsi ppp_generic slhc nfnetlink can ipt_ULOG ax25 decnet irda nfc rds x25 crc_ccitt appletalk atm ipx p8023 psnap p8022 llc lockd sunrpc ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_conntrack nf_conntrack ip6table_filter ip6_tables btusb bluetooth snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_pcm vhost_net snd_page_alloc snd_timer tun macvtap usb_debug snd rfkill microcode macvlan edac_core pcspkr serio_raw kvm_amd soundcore kvm r8169 mii [ 89.637846] CPU 2 [ 89.638175] Pid: 782, comm: trinity-main Not tainted 3.8.0+ #63 Gigabyte Technology Co., Ltd. GA-MA78GM-S2H/GA-MA78GM-S2H [ 89.639850] RIP: 0010:[] [] commit_creds+0x250/0x2f0 [ 89.641161] RSP: 0018:880115657eb8 EFLAGS: 00010207 [ 89.641984] RAX: 03e8 RBX: 88012688b000 RCX: [ 89.643069] RDX: RSI: 81c32960 RDI: 880105839600 [ 89.644167] RBP: 880115657ed8 R08: R09: [ 89.645254] R10: 0001 R11: 0246 R12: 880105839600 [ 89.646340] R13: 88011beea490 R14: 88011beea490 R15: [ 89.647431] FS: 7f3ac063b740() GS:88012b20() knlGS: [ 89.648660] CS: 0010 DS: ES: CR0: 8005003b [ 89.649548] CR2: 00c8 CR3: 000122bfc000 CR4: 07e0 [ 89.650635] DR0: DR1: DR2: [ 89.651723] DR3: DR6: 0ff0 DR7: 0400 [ 89.652812] Process trinity-main (pid: 782, threadinfo 880115656000, task 88011beea490) [ 89.654128] Stack: [ 89.654433] 8801058396a0 880105839600 88011beeaa78 [ 89.655769] 880115657ef8 812c7d9b 82079be0 [ 89.657073] 880115657f28 8106c665 0002 880115657f58 [ 89.658399] Call Trace: [ 89.658822] [] key_change_session_keyring+0xfb/0x140 [ 89.659845] [] task_work_run+0xa5/0xd0 [ 89.660698] [] do_notify_resume+0x71/0xb0 [ 89.661581] [] int_signal+0x12/0x17 [ 89.662385] Code: 24 90 00 00 00 48 8b b3 90 00 00 00 49 8b 4c 24 40 48 39 f2 75 08 e9 83 00 00 00 48 89 ca 48 81 fa 60 29 c3 81 0f 84 41 fe ff ff <48> 8b 8a c8 00 00 00 48 39 ce 75 e4 3b 82 d0 00 00 00 0f 84 4b [ 89.667778] RIP [] commit_creds+0x250/0x2f0 [ 89.668733] RSP [ 89.669301] CR2: 00c8 My fastest trinity induced oops yet! Appears to be.. if ((set_ns == subset_ns->parent) && 850: 48 8b 8a c8 00 00 00mov0xc8(%rdx),%rcx from the inlined cred_cap_issubset Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
commit_creds oops
Just hit this on Linus' current tree. [ 89.621770] BUG: unable to handle kernel NULL pointer dereference at 00c8 [ 89.623111] IP: [810784b0] commit_creds+0x250/0x2f0 [ 89.624062] PGD 122bfd067 PUD 122bfe067 PMD 0 [ 89.624901] Oops: [#1] PREEMPT SMP [ 89.625678] Modules linked in: caif_socket caif netrom bridge hidp 8021q garp stp mrp rose llc2 af_rxrpc phonet af_key binfmt_misc bnep l2tp_ppp can_bcm l2tp_core pppoe pppox can_raw scsi_transport_iscsi ppp_generic slhc nfnetlink can ipt_ULOG ax25 decnet irda nfc rds x25 crc_ccitt appletalk atm ipx p8023 psnap p8022 llc lockd sunrpc ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_conntrack nf_conntrack ip6table_filter ip6_tables btusb bluetooth snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_pcm vhost_net snd_page_alloc snd_timer tun macvtap usb_debug snd rfkill microcode macvlan edac_core pcspkr serio_raw kvm_amd soundcore kvm r8169 mii [ 89.637846] CPU 2 [ 89.638175] Pid: 782, comm: trinity-main Not tainted 3.8.0+ #63 Gigabyte Technology Co., Ltd. GA-MA78GM-S2H/GA-MA78GM-S2H [ 89.639850] RIP: 0010:[810784b0] [810784b0] commit_creds+0x250/0x2f0 [ 89.641161] RSP: 0018:880115657eb8 EFLAGS: 00010207 [ 89.641984] RAX: 03e8 RBX: 88012688b000 RCX: [ 89.643069] RDX: RSI: 81c32960 RDI: 880105839600 [ 89.644167] RBP: 880115657ed8 R08: R09: [ 89.645254] R10: 0001 R11: 0246 R12: 880105839600 [ 89.646340] R13: 88011beea490 R14: 88011beea490 R15: [ 89.647431] FS: 7f3ac063b740() GS:88012b20() knlGS: [ 89.648660] CS: 0010 DS: ES: CR0: 8005003b [ 89.649548] CR2: 00c8 CR3: 000122bfc000 CR4: 07e0 [ 89.650635] DR0: DR1: DR2: [ 89.651723] DR3: DR6: 0ff0 DR7: 0400 [ 89.652812] Process trinity-main (pid: 782, threadinfo 880115656000, task 88011beea490) [ 89.654128] Stack: [ 89.654433] 8801058396a0 880105839600 88011beeaa78 [ 89.655769] 880115657ef8 812c7d9b 82079be0 [ 89.657073] 880115657f28 8106c665 0002 880115657f58 [ 89.658399] Call Trace: [ 89.658822] [812c7d9b] key_change_session_keyring+0xfb/0x140 [ 89.659845] [8106c665] task_work_run+0xa5/0xd0 [ 89.660698] [81002911] do_notify_resume+0x71/0xb0 [ 89.661581] [816c9a4a] int_signal+0x12/0x17 [ 89.662385] Code: 24 90 00 00 00 48 8b b3 90 00 00 00 49 8b 4c 24 40 48 39 f2 75 08 e9 83 00 00 00 48 89 ca 48 81 fa 60 29 c3 81 0f 84 41 fe ff ff 48 8b 8a c8 00 00 00 48 39 ce 75 e4 3b 82 d0 00 00 00 0f 84 4b [ 89.667778] RIP [810784b0] commit_creds+0x250/0x2f0 [ 89.668733] RSP 880115657eb8 [ 89.669301] CR2: 00c8 My fastest trinity induced oops yet! Appears to be.. if ((set_ns == subset_ns-parent) 850: 48 8b 8a c8 00 00 00mov0xc8(%rdx),%rcx from the inlined cred_cap_issubset Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: commit_creds oops
Dave Jones da...@redhat.com writes: Just hit this on Linus' current tree. [ 89.621770] BUG: unable to handle kernel NULL pointer dereference at 00c8 [ 89.623111] IP: [810784b0] commit_creds+0x250/0x2f0 [ 89.624062] PGD 122bfd067 PUD 122bfe067 PMD 0 [ 89.624901] Oops: [#1] PREEMPT SMP [ 89.625678] Modules linked in: caif_socket caif netrom bridge hidp 8021q garp stp mrp rose llc2 af_rxrpc phonet af_key binfmt_misc bnep l2tp_ppp can_bcm l2tp_core pppoe pppox can_raw scsi_transport_iscsi ppp_generic slhc nfnetlink can ipt_ULOG ax25 decnet irda nfc rds x25 crc_ccitt appletalk atm ipx p8023 psnap p8022 llc lockd sunrpc ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_conntrack nf_conntrack ip6table_filter ip6_tables btusb bluetooth snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_pcm vhost_net snd_page_alloc snd_timer tun macvtap usb_debug snd rfkill microcode macvlan edac_core pcspkr serio_raw kvm_amd soundcore kvm r8169 mii [ 89.637846] CPU 2 [ 89.638175] Pid: 782, comm: trinity-main Not tainted 3.8.0+ #63 Gigabyte Technology Co., Ltd. GA-MA78GM-S2H/GA-MA78GM-S2H [ 89.639850] RIP: 0010:[810784b0] [810784b0] commit_creds+0x250/0x2f0 [ 89.641161] RSP: 0018:880115657eb8 EFLAGS: 00010207 [ 89.641984] RAX: 03e8 RBX: 88012688b000 RCX: [ 89.643069] RDX: RSI: 81c32960 RDI: 880105839600 [ 89.644167] RBP: 880115657ed8 R08: R09: [ 89.645254] R10: 0001 R11: 0246 R12: 880105839600 [ 89.646340] R13: 88011beea490 R14: 88011beea490 R15: [ 89.647431] FS: 7f3ac063b740() GS:88012b20() knlGS: [ 89.648660] CS: 0010 DS: ES: CR0: 8005003b [ 89.649548] CR2: 00c8 CR3: 000122bfc000 CR4: 07e0 [ 89.650635] DR0: DR1: DR2: [ 89.651723] DR3: DR6: 0ff0 DR7: 0400 [ 89.652812] Process trinity-main (pid: 782, threadinfo 880115656000, task 88011beea490) [ 89.654128] Stack: [ 89.654433] 8801058396a0 880105839600 88011beeaa78 [ 89.655769] 880115657ef8 812c7d9b 82079be0 [ 89.657073] 880115657f28 8106c665 0002 880115657f58 [ 89.658399] Call Trace: [ 89.658822] [812c7d9b] key_change_session_keyring+0xfb/0x140 [ 89.659845] [8106c665] task_work_run+0xa5/0xd0 [ 89.660698] [81002911] do_notify_resume+0x71/0xb0 [ 89.661581] [816c9a4a] int_signal+0x12/0x17 [ 89.662385] Code: 24 90 00 00 00 48 8b b3 90 00 00 00 49 8b 4c 24 40 48 39 f2 75 08 e9 83 00 00 00 48 89 ca 48 81 fa 60 29 c3 81 0f 84 41 fe ff ff 48 8b 8a c8 00 00 00 48 39 ce 75 e4 3b 82 d0 00 00 00 0f 84 4b [ 89.667778] RIP [810784b0] commit_creds+0x250/0x2f0 [ 89.668733] RSP 880115657eb8 [ 89.669301] CR2: 00c8 My fastest trinity induced oops yet! Appears to be.. if ((set_ns == subset_ns-parent) 850: 48 8b 8a c8 00 00 00mov0xc8(%rdx),%rcx from the inlined cred_cap_issubset Interesting. That line is protected with the check subset_ns != init_user_ns so subset_ns-parent must be valid or subset_ns is not a proper user namespace. Ugh. I think I see what is going on and it is just silly. It looks like by historical accident we have been reading trying to set new-user_ns from new-user_ns. Which is totally silly as new-user_ns is NULL (as is every other field in new except session_keyring at that point). It looks like it is safe to sleep in key_change_session_keyring so why we just don't use prepare_creds there like everywhere else is beyond me. The intent is clearly to copy all of the fields from old to new so what we should be doing is is copying old-user_ns into new-user_ns. Dave can you verify that this patch fixes the oops? Signed-off-by: Eric W. Biederman ebied...@xmission.com --- diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c index 58dfe08..a571fad 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -839,7 +839,7 @@ void key_change_session_keyring(struct callback_head *twork) new- sgid = old- sgid; new-fsgid = old-fsgid; new-user = get_uid(old-user); - new-user_ns= get_user_ns(new-user_ns); + new-user_ns= get_user_ns(old-user_ns); new-group_info = get_group_info(old-group_info); new-securebits = old-securebits; -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the
Re: commit_creds oops
Quoting Eric W. Biederman (ebied...@xmission.com): Dave Jones da...@redhat.com writes: Just hit this on Linus' current tree. [ 89.621770] BUG: unable to handle kernel NULL pointer dereference at 00c8 [ 89.623111] IP: [810784b0] commit_creds+0x250/0x2f0 [ 89.624062] PGD 122bfd067 PUD 122bfe067 PMD 0 [ 89.624901] Oops: [#1] PREEMPT SMP [ 89.625678] Modules linked in: caif_socket caif netrom bridge hidp 8021q garp stp mrp rose llc2 af_rxrpc phonet af_key binfmt_misc bnep l2tp_ppp can_bcm l2tp_core pppoe pppox can_raw scsi_transport_iscsi ppp_generic slhc nfnetlink can ipt_ULOG ax25 decnet irda nfc rds x25 crc_ccitt appletalk atm ipx p8023 psnap p8022 llc lockd sunrpc ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_conntrack nf_conntrack ip6table_filter ip6_tables btusb bluetooth snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_pcm vhost_net snd_page_alloc snd_timer tun macvtap usb_debug snd rfkill microcode macvlan edac_core pcspkr serio_raw kvm_amd soundcore kvm r8169 mii [ 89.637846] CPU 2 [ 89.638175] Pid: 782, comm: trinity-main Not tainted 3.8.0+ #63 Gigabyte Technology Co., Ltd. GA-MA78GM-S2H/GA-MA78GM-S2H [ 89.639850] RIP: 0010:[810784b0] [810784b0] commit_creds+0x250/0x2f0 [ 89.641161] RSP: 0018:880115657eb8 EFLAGS: 00010207 [ 89.641984] RAX: 03e8 RBX: 88012688b000 RCX: [ 89.643069] RDX: RSI: 81c32960 RDI: 880105839600 [ 89.644167] RBP: 880115657ed8 R08: R09: [ 89.645254] R10: 0001 R11: 0246 R12: 880105839600 [ 89.646340] R13: 88011beea490 R14: 88011beea490 R15: [ 89.647431] FS: 7f3ac063b740() GS:88012b20() knlGS: [ 89.648660] CS: 0010 DS: ES: CR0: 8005003b [ 89.649548] CR2: 00c8 CR3: 000122bfc000 CR4: 07e0 [ 89.650635] DR0: DR1: DR2: [ 89.651723] DR3: DR6: 0ff0 DR7: 0400 [ 89.652812] Process trinity-main (pid: 782, threadinfo 880115656000, task 88011beea490) [ 89.654128] Stack: [ 89.654433] 8801058396a0 880105839600 88011beeaa78 [ 89.655769] 880115657ef8 812c7d9b 82079be0 [ 89.657073] 880115657f28 8106c665 0002 880115657f58 [ 89.658399] Call Trace: [ 89.658822] [812c7d9b] key_change_session_keyring+0xfb/0x140 [ 89.659845] [8106c665] task_work_run+0xa5/0xd0 [ 89.660698] [81002911] do_notify_resume+0x71/0xb0 [ 89.661581] [816c9a4a] int_signal+0x12/0x17 [ 89.662385] Code: 24 90 00 00 00 48 8b b3 90 00 00 00 49 8b 4c 24 40 48 39 f2 75 08 e9 83 00 00 00 48 89 ca 48 81 fa 60 29 c3 81 0f 84 41 fe ff ff 48 8b 8a c8 00 00 00 48 39 ce 75 e4 3b 82 d0 00 00 00 0f 84 4b [ 89.667778] RIP [810784b0] commit_creds+0x250/0x2f0 [ 89.668733] RSP 880115657eb8 [ 89.669301] CR2: 00c8 My fastest trinity induced oops yet! Appears to be.. if ((set_ns == subset_ns-parent) 850: 48 8b 8a c8 00 00 00mov0xc8(%rdx),%rcx from the inlined cred_cap_issubset Interesting. That line is protected with the check subset_ns != init_user_ns so subset_ns-parent must be valid or subset_ns is not a proper user namespace. Ugh. I think I see what is going on and it is just silly. It looks like by historical accident we have been reading trying to set new-user_ns from new-user_ns. Which is totally silly as new-user_ns is NULL (as is every other field in new except session_keyring at that point). It looks like it is safe to sleep in key_change_session_keyring so why we just don't use prepare_creds there like everywhere else is beyond me. The intent is clearly to copy all of the fields from old to new so what we should be doing is is copying old-user_ns into new-user_ns. Dave can you verify that this patch fixes the oops? Signed-off-by: Eric W. Biederman ebied...@xmission.com Jinkeys - that should have stood out like a sore thumb. Acked-by: Serge Hallyn serge.hal...@canonical.com --- diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c index 58dfe08..a571fad 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -839,7 +839,7 @@ void key_change_session_keyring(struct callback_head *twork) new- sgid = old- sgid; new-fsgid = old-fsgid; new-user = get_uid(old-user); - new-user_ns= get_user_ns(new-user_ns); + new-user_ns= get_user_ns(old-user_ns); new-group_info =
Re: commit_creds oops
On Thu, Feb 28, 2013 at 04:25:40PM -0800, Eric W. Biederman wrote: [ 89.639850] RIP: 0010:[810784b0] [810784b0] commit_creds+0x250/0x2f0 [ 89.658399] Call Trace: [ 89.658822] [812c7d9b] key_change_session_keyring+0xfb/0x140 [ 89.659845] [8106c665] task_work_run+0xa5/0xd0 [ 89.660698] [81002911] do_notify_resume+0x71/0xb0 [ 89.661581] [816c9a4a] int_signal+0x12/0x17 Appears to be.. if ((set_ns == subset_ns-parent) 850: 48 8b 8a c8 00 00 00mov0xc8(%rdx),%rcx from the inlined cred_cap_issubset Interesting. That line is protected with the check subset_ns != init_user_ns so subset_ns-parent must be valid or subset_ns is not a proper user namespace. Ugh. I think I see what is going on and it is just silly. It looks like by historical accident we have been reading trying to set new-user_ns from new-user_ns. Which is totally silly as new-user_ns is NULL (as is every other field in new except session_keyring at that point). It looks like it is safe to sleep in key_change_session_keyring so why we just don't use prepare_creds there like everywhere else is beyond me. The intent is clearly to copy all of the fields from old to new so what we should be doing is is copying old-user_ns into new-user_ns. Dave can you verify that this patch fixes the oops? Looks like it. Haven't hit the same thing since applying your patch. I noticed though that get_user_ns bumps a refcount. Is this what we want if we're just copying ? Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: commit_creds oops
Dave Jones da...@redhat.com writes: On Thu, Feb 28, 2013 at 04:25:40PM -0800, Eric W. Biederman wrote: [ 89.639850] RIP: 0010:[810784b0] [810784b0] commit_creds+0x250/0x2f0 [ 89.658399] Call Trace: [ 89.658822] [812c7d9b] key_change_session_keyring+0xfb/0x140 [ 89.659845] [8106c665] task_work_run+0xa5/0xd0 [ 89.660698] [81002911] do_notify_resume+0x71/0xb0 [ 89.661581] [816c9a4a] int_signal+0x12/0x17 Appears to be.. if ((set_ns == subset_ns-parent) 850: 48 8b 8a c8 00 00 00mov0xc8(%rdx),%rcx from the inlined cred_cap_issubset Interesting. That line is protected with the check subset_ns != init_user_ns so subset_ns-parent must be valid or subset_ns is not a proper user namespace. Ugh. I think I see what is going on and it is just silly. It looks like by historical accident we have been reading trying to set new-user_ns from new-user_ns. Which is totally silly as new-user_ns is NULL (as is every other field in new except session_keyring at that point). It looks like it is safe to sleep in key_change_session_keyring so why we just don't use prepare_creds there like everywhere else is beyond me. The intent is clearly to copy all of the fields from old to new so what we should be doing is is copying old-user_ns into new-user_ns. Dave can you verify that this patch fixes the oops? Looks like it. Haven't hit the same thing since applying your patch. I noticed though that get_user_ns bumps a refcount. Is this what we want if we're just copying ? Yes. commit_creds(new) winds up finding old on the current process and calling put_cred(old). put_cred when the count drops to zero winds up calling put_cred_rcu which calls put_user_ns(old-user_ns); For the same reason we need an extra count on the user namespace new so that when it eventually is put and put_user_ns(new-user_ns) is called we don't have a negative count. Which is a long of way of saying yes we are adding another reference and we need to increase the reference count. Eric -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
