Message ID | 1339425693.6001.2268.camel@edumazet-glaptop |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On Mon, 2012-06-11 at 16:41 +0200, Eric Dumazet wrote: > On Mon, 2012-06-11 at 16:41 +0200, Samuel Ortiz wrote: > > Hi Sasha, > > > > On Mon, Jun 11, 2012 at 04:00:41PM +0200, Sasha Levin wrote: > > > Hi all, > > > > > > I've stumbled on the following while fuzzing with trinity inside a KVM tools guest, running on 3.5-rc2: > > > > > Thanks for the report, it could be worth adding this one to > > bugzilla.kernel.org. > > > > What's trinity ? > > Also, if this one is reproducible, would you mind sharing some details about > > how we could reproduce it ? > > Well, bugfix should be trivial enough ;) > > diff --git a/net/nfc/rawsock.c b/net/nfc/rawsock.c > index ec1134c..208416e 100644 > --- a/net/nfc/rawsock.c > +++ b/net/nfc/rawsock.c > @@ -54,11 +54,12 @@ static int rawsock_release(struct socket *sock) > { > struct sock *sk = sock->sk; > > - pr_debug("sock=%p\n", sock); > - > - sock_orphan(sk); > - sock_put(sk); > + pr_debug("sock=%p sk=%p\n", sock, sk); > > + if (sk) { > + sock_orphan(sk); > + sock_put(sk); > + } > return 0; > } Eric, Is there something that documents at what state each of the callbacks in the network subsystem can be called? Like a big flow chart of some sorts? I'm asking because I've looked at this as well before sending this mail, and while the fix does look trivial, I wasn't sure whether it is really the correct fix, or the problem is that this callback wasn't supposed be called at all so something else is broken (we had such issue with namespaces and unshare() not long ago). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Eric, On Mon, Jun 11, 2012 at 04:41:33PM +0200, Eric Dumazet wrote: > On Mon, 2012-06-11 at 16:41 +0200, Samuel Ortiz wrote: > > Hi Sasha, > > > > On Mon, Jun 11, 2012 at 04:00:41PM +0200, Sasha Levin wrote: > > > Hi all, > > > > > > I've stumbled on the following while fuzzing with trinity inside a KVM tools guest, running on 3.5-rc2: > > > > > Thanks for the report, it could be worth adding this one to > > bugzilla.kernel.org. > > > > What's trinity ? > > Also, if this one is reproducible, would you mind sharing some details about > > how we could reproduce it ? > > Well, bugfix should be trivial enough ;) Yep, I looked at the code only after looking at Sasha's report. Thanks for the patch, do you mind if I add your SOB to it ? Cheers, Samuel.
On Mon, 2012-06-11 at 16:50 +0200, Sasha Levin wrote: > Eric, Is there something that documents at what state each of the > callbacks in the network subsystem can be called? Like a big flow chart > of some sorts? > > I'm asking because I've looked at this as well before sending this mail, > and while the fix does look trivial, I wasn't sure whether it is really > the correct fix, or the problem is that this callback wasn't supposed be > called at all so something else is broken (we had such issue with > namespaces and unshare() not long ago). > I am not aware of such 'document'. Things change, and only *good* reference is actual source code. Now, take a look at sock_graft()/sock_orphan()/inet_release() ... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-06-11 at 16:57 +0200, Samuel Ortiz wrote: > Hi Eric, > > On Mon, Jun 11, 2012 at 04:41:33PM +0200, Eric Dumazet wrote: > > On Mon, 2012-06-11 at 16:41 +0200, Samuel Ortiz wrote: > > > Hi Sasha, > > > > > > On Mon, Jun 11, 2012 at 04:00:41PM +0200, Sasha Levin wrote: > > > > Hi all, > > > > > > > > I've stumbled on the following while fuzzing with trinity inside a KVM tools guest, running on 3.5-rc2: > > > > > > > Thanks for the report, it could be worth adding this one to > > > bugzilla.kernel.org. > > > > > > What's trinity ? > > > Also, if this one is reproducible, would you mind sharing some details about > > > how we could reproduce it ? > > > > Well, bugfix should be trivial enough ;) > Yep, I looked at the code only after looking at Sasha's report. > > Thanks for the patch, do you mind if I add your SOB to it ? I would prefer making sure it fixes the bug first ;) Thanks -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 11, 2012 at 04:59:38PM +0200, Eric Dumazet wrote: > On Mon, 2012-06-11 at 16:57 +0200, Samuel Ortiz wrote: > > Hi Eric, > > > > On Mon, Jun 11, 2012 at 04:41:33PM +0200, Eric Dumazet wrote: > > > On Mon, 2012-06-11 at 16:41 +0200, Samuel Ortiz wrote: > > > > Hi Sasha, > > > > > > > > On Mon, Jun 11, 2012 at 04:00:41PM +0200, Sasha Levin wrote: > > > > > Hi all, > > > > > > > > > > I've stumbled on the following while fuzzing with trinity inside a KVM tools guest, running on 3.5-rc2: > > > > > > > > > Thanks for the report, it could be worth adding this one to > > > > bugzilla.kernel.org. > > > > > > > > What's trinity ? > > > > Also, if this one is reproducible, would you mind sharing some details about > > > > how we could reproduce it ? > > > > > > Well, bugfix should be trivial enough ;) > > Yep, I looked at the code only after looking at Sasha's report. > > > > Thanks for the patch, do you mind if I add your SOB to it ? > > I would prefer making sure it fixes the bug first ;) Sure, although your patch makes sense regardless of that :) I'll still wait for Sasha to confirm that it fixes his crash. Cheers, Samuel.
On Mon, 2012-06-11 at 16:58 +0200, Eric Dumazet wrote: > On Mon, 2012-06-11 at 16:50 +0200, Sasha Levin wrote: > > > Eric, Is there something that documents at what state each of the > > callbacks in the network subsystem can be called? Like a big flow chart > > of some sorts? > > > > I'm asking because I've looked at this as well before sending this mail, > > and while the fix does look trivial, I wasn't sure whether it is really > > the correct fix, or the problem is that this callback wasn't supposed be > > called at all so something else is broken (we had such issue with > > namespaces and unshare() not long ago). > > > > I am not aware of such 'document'. > > Things change, and only *good* reference is actual source code. > > Now, take a look at sock_graft()/sock_orphan()/inet_release() ... I see. I grepped for release callbacks and the first few in the result (atm, ax_25) did check for !sk, so I guess I'll just follow what I see in other code in the future :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-06-11 at 17:20 +0200, Samuel Ortiz wrote: > On Mon, Jun 11, 2012 at 04:59:38PM +0200, Eric Dumazet wrote: > > On Mon, 2012-06-11 at 16:57 +0200, Samuel Ortiz wrote: > > > Hi Eric, > > > > > > On Mon, Jun 11, 2012 at 04:41:33PM +0200, Eric Dumazet wrote: > > > > On Mon, 2012-06-11 at 16:41 +0200, Samuel Ortiz wrote: > > > > > Hi Sasha, > > > > > > > > > > On Mon, Jun 11, 2012 at 04:00:41PM +0200, Sasha Levin wrote: > > > > > > Hi all, > > > > > > > > > > > > I've stumbled on the following while fuzzing with trinity inside a KVM tools guest, running on 3.5-rc2: > > > > > > > > > > > Thanks for the report, it could be worth adding this one to > > > > > bugzilla.kernel.org. > > > > > > > > > > What's trinity ? > > > > > Also, if this one is reproducible, would you mind sharing some details about > > > > > how we could reproduce it ? > > > > > > > > Well, bugfix should be trivial enough ;) > > > Yep, I looked at the code only after looking at Sasha's report. > > > > > > Thanks for the patch, do you mind if I add your SOB to it ? > > > > I would prefer making sure it fixes the bug first ;) > Sure, although your patch makes sense regardless of that :) > I'll still wait for Sasha to confirm that it fixes his crash. I don't have a direct way of reproducing it, but I've put it in the test vm and the fuzzer is running, I'll let you know tomorrow if it happened again. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 11, 2012 at 06:56:50PM +0200, Sasha Levin wrote: > > > > > > What's trinity ? > > > > > > Also, if this one is reproducible, would you mind sharing some details about > > > > > > how we could reproduce it ? > > > > > > > > > > Well, bugfix should be trivial enough ;) > > > > Yep, I looked at the code only after looking at Sasha's report. > > > > > > > > Thanks for the patch, do you mind if I add your SOB to it ? > > > > > > I would prefer making sure it fixes the bug first ;) > > Sure, although your patch makes sense regardless of that :) > > I'll still wait for Sasha to confirm that it fixes his crash. > > I don't have a direct way of reproducing it, but I've put it in the test > vm and the fuzzer is running, I'll let you know tomorrow if it happened > again. You might be able to trigger it faster by using -P PF_NFC, which will force trinity to only use NFC sockets. sidenote: most protocols trigger the module to be auto-loaded when a socket is created. This doesn't seem to happen with nfc, making me need to manually modprobe it first. Intentional ? Dave -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dave, On Mon, Jun 11, 2012 at 01:25:45PM -0400, Dave Jones wrote: > On Mon, Jun 11, 2012 at 06:56:50PM +0200, Sasha Levin wrote: > > > > > > > > What's trinity ? > > > > > > > Also, if this one is reproducible, would you mind sharing some details about > > > > > > > how we could reproduce it ? > > > > > > > > > > > > Well, bugfix should be trivial enough ;) > > > > > Yep, I looked at the code only after looking at Sasha's report. > > > > > > > > > > Thanks for the patch, do you mind if I add your SOB to it ? > > > > > > > > I would prefer making sure it fixes the bug first ;) > > > Sure, although your patch makes sense regardless of that :) > > > I'll still wait for Sasha to confirm that it fixes his crash. > > > > I don't have a direct way of reproducing it, but I've put it in the test > > vm and the fuzzer is running, I'll let you know tomorrow if it happened > > again. > > You might be able to trigger it faster by using -P PF_NFC, which will > force trinity to only use NFC sockets. > > sidenote: most protocols trigger the module to be auto-loaded when a socket > is created. This doesn't seem to happen with nfc, making me need to manually > modprobe it first. Intentional ? No, I'm missing the MODULE_ALIAS_NETPROTO() call for NFC. Thanks for the report. Cheers, Samuel.
On Mon, Jun 11, 2012 at 7:25 PM, Dave Jones <davej@redhat.com> wrote: > On Mon, Jun 11, 2012 at 06:56:50PM +0200, Sasha Levin wrote: > > > > > > > > What's trinity ? > > > > > > > Also, if this one is reproducible, would you mind sharing some details about > > > > > > > how we could reproduce it ? > > > > > > > > > > > > Well, bugfix should be trivial enough ;) > > > > > Yep, I looked at the code only after looking at Sasha's report. > > > > > > > > > > Thanks for the patch, do you mind if I add your SOB to it ? > > > > > > > > I would prefer making sure it fixes the bug first ;) > > > Sure, although your patch makes sense regardless of that :) > > > I'll still wait for Sasha to confirm that it fixes his crash. > > > > I don't have a direct way of reproducing it, but I've put it in the test > > vm and the fuzzer is running, I'll let you know tomorrow if it happened > > again. > > You might be able to trigger it faster by using -P PF_NFC, which will > force trinity to only use NFC sockets. > > sidenote: most protocols trigger the module to be auto-loaded when a socket > is created. This doesn't seem to happen with nfc, making me need to manually > modprobe it first. Intentional ? It fixes the bug, wasn't been able to reproduce it since then. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/nfc/rawsock.c b/net/nfc/rawsock.c index ec1134c..208416e 100644 --- a/net/nfc/rawsock.c +++ b/net/nfc/rawsock.c @@ -54,11 +54,12 @@ static int rawsock_release(struct socket *sock) { struct sock *sk = sock->sk; - pr_debug("sock=%p\n", sock); - - sock_orphan(sk); - sock_put(sk); + pr_debug("sock=%p sk=%p\n", sock, sk); + if (sk) { + sock_orphan(sk); + sock_put(sk); + } return 0; }