| Message ID | 001c01d1849b$1c7c98e0$5575caa0$@codeaurora.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Mar 22, 2016 at 06:29:48PM -0600, Subash Abhinov Kasiviswanathan wrote: > A crash is observed when a decrypted packet is processed in receive > path. get_rps_cpus() tries to dereference the skb->dev fields but it > appears that the device is freed from the poison pattern. > > [<ffffffc000af58ec>] get_rps_cpu+0x94/0x2f0 > [<ffffffc000af5f94>] netif_rx_internal+0x140/0x1cc > [<ffffffc000af6094>] netif_rx+0x74/0x94 > [<ffffffc000bc0b6c>] xfrm_input+0x754/0x7d0 > [<ffffffc000bc0bf8>] xfrm_input_resume+0x10/0x1c > [<ffffffc000ba6eb8>] esp_input_done+0x20/0x30 > [<ffffffc0000b64c8>] process_one_work+0x244/0x3fc > [<ffffffc0000b7324>] worker_thread+0x2f8/0x418 > [<ffffffc0000bb40c>] kthread+0xe0/0xec > > -013|get_rps_cpu( > | dev = 0xFFFFFFC08B688000, > | skb = 0xFFFFFFC0C76AAC00 -> ( > | dev = 0xFFFFFFC08B688000 -> ( > | name = > "...................................................... > | name_hlist = (next = 0xAAAAAAAAAAAAAAAA, pprev = > 0xAAAAAAAAAAA > > Following are the sequence of events observed - > > - Encrypted packet in receive path from netdevice is queued > - Encrypted packet queued for decryption (asynchronous) > - Netdevice brought down and freed > - Packet is decrypted and returned through callback in esp_input_done > - Packet is queued again for process in network stack using netif_rx > > Since the device appears to have been freed, the dereference of > skb->dev in get_rps_cpus() leads to an unhandled page fault > exception. > > Fix this by holding on to device reference when queueing packets > asynchronously and releasing the reference on call back return. > > v2: Make the change generic to xfrm as mentioned by Steffen and > update the title to xfrm > > Suggested-by: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Jerome Stanislaus <jeromes@codeaurora.org> > Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> Looks good. David, in case you want to take it directly into the net tree: Acked-by: Steffen Klassert <steffen.klassert@secunet.com>
On Tue, 2016-03-22 at 18:29 -0600, Subash Abhinov Kasiviswanathan wrote: > A crash is observed when a decrypted packet is processed in receive > path. get_rps_cpus() tries to dereference the skb->dev fields but it > appears that the device is freed from the poison pattern. > > [<ffffffc000af58ec>] get_rps_cpu+0x94/0x2f0 > [<ffffffc000af5f94>] netif_rx_internal+0x140/0x1cc > [<ffffffc000af6094>] netif_rx+0x74/0x94 > [<ffffffc000bc0b6c>] xfrm_input+0x754/0x7d0 > [<ffffffc000bc0bf8>] xfrm_input_resume+0x10/0x1c > [<ffffffc000ba6eb8>] esp_input_done+0x20/0x30 > [<ffffffc0000b64c8>] process_one_work+0x244/0x3fc > [<ffffffc0000b7324>] worker_thread+0x2f8/0x418 > [<ffffffc0000bb40c>] kthread+0xe0/0xec > > -013|get_rps_cpu( > | dev = 0xFFFFFFC08B688000, > | skb = 0xFFFFFFC0C76AAC00 -> ( > | dev = 0xFFFFFFC08B688000 -> ( > | name = > "...................................................... > | name_hlist = (next = 0xAAAAAAAAAAAAAAAA, pprev = > 0xAAAAAAAAAAA > > Following are the sequence of events observed - > > - Encrypted packet in receive path from netdevice is queued > - Encrypted packet queued for decryption (asynchronous) > - Netdevice brought down and freed > - Packet is decrypted and returned through callback in esp_input_done > - Packet is queued again for process in network stack using netif_rx > > Since the device appears to have been freed, the dereference of > skb->dev in get_rps_cpus() leads to an unhandled page fault > exception. > > Fix this by holding on to device reference when queueing packets > asynchronously and releasing the reference on call back return. > > v2: Make the change generic to xfrm as mentioned by Steffen and > update the title to xfrm > > Suggested-by: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Jerome Stanislaus <jeromes@codeaurora.org> > Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> > --- > net/xfrm/xfrm_input.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c > index ad7f5b3..1c4ad47 100644 > --- a/net/xfrm/xfrm_input.c > +++ b/net/xfrm/xfrm_input.c > @@ -292,12 +292,15 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 > spi, int encap_type) > XFRM_SKB_CB(skb)->seq.input.hi = seq_hi; > > skb_dst_force(skb); > + dev_hold(skb->dev); > > nexthdr = x->type->input(x, skb); > > if (nexthdr == -EINPROGRESS) > return 0; > resume: > + dev_put(skb->dev); > + > spin_lock(&x->lock); > if (nexthdr <= 0) { > if (nexthdr == -EBADMSG) { > -- > Wont this prevent device from being dismantled ? Where is this xfrm queue purged at device dismantle ? dev_put() is probably missing, if you add a dev_hold() for every packet in it.
On Wed, Mar 23, 2016 at 06:25:47AM -0700, Eric Dumazet wrote: > > Wont this prevent device from being dismantled ? This is only held while the crypto processing is ongoing. > Where is this xfrm queue purged at device dismantle ? There is no way to cancel an ongoing crypto processing so you'll just have to wait it out. Cheers,
On Wed, 2016-03-23 at 21:29 +0800, Herbert Xu wrote: > On Wed, Mar 23, 2016 at 06:25:47AM -0700, Eric Dumazet wrote: > > > > Wont this prevent device from being dismantled ? > > This is only held while the crypto processing is ongoing. > > > Where is this xfrm queue purged at device dismantle ? > > There is no way to cancel an ongoing crypto processing so you'll > just have to wait it out. OK, but before calling netif_rx() are we properly testing dev->flags IFF_UP status ? Otherwise, we still allow packets being queued after flush_backlog() had been called. Thanks.
On Wed, Mar 23, 2016 at 10:29:25AM -0700, Eric Dumazet wrote: > > OK, but before calling netif_rx() are we properly testing dev->flags > IFF_UP status ? > > Otherwise, we still allow packets being queued after flush_backlog() had > been called. That's the first thing enqueue_to_backlog tests. Cheers,
On Thu, 2016-03-24 at 08:45 +0800, Herbert Xu wrote: > On Wed, Mar 23, 2016 at 10:29:25AM -0700, Eric Dumazet wrote: > > > > OK, but before calling netif_rx() are we properly testing dev->flags > > IFF_UP status ? > > > > Otherwise, we still allow packets being queued after flush_backlog() had > > been called. > > That's the first thing enqueue_to_backlog tests. > > Cheers, Seems to be very recent stuff ( commit e9e4dd3267d0c5234c5c0f47440456b10875dec9 in linux-4.2) In the old days the test was done in callers, since in most cases NIC drivers do not need it. Lets make sure this was backported to all stable trees. And then we probably can cleanup some callers as well.
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 23 Mar 2016 18:39:57 -0700 > On Thu, 2016-03-24 at 08:45 +0800, Herbert Xu wrote: >> On Wed, Mar 23, 2016 at 10:29:25AM -0700, Eric Dumazet wrote: >> > >> > OK, but before calling netif_rx() are we properly testing dev->flags >> > IFF_UP status ? >> > >> > Otherwise, we still allow packets being queued after flush_backlog() had >> > been called. >> >> That's the first thing enqueue_to_backlog tests. >> >> Cheers, > > Seems to be very recent stuff ( commit > e9e4dd3267d0c5234c5c0f47440456b10875dec9 in linux-4.2) > > In the old days the test was done in callers, since in most cases NIC > drivers do not need it. > > Lets make sure this was backported to all stable trees. > > And then we probably can cleanup some callers as well. Anyways this patch needs to be redone because it is corrupted by the submitter's email client. I'll queue it up and make sure e9e4dd3267d0c5234c5c0f47440456b10875dec9 ends up in -stable where needed. Thanks.
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c index ad7f5b3..1c4ad47 100644 --- a/net/xfrm/xfrm_input.c +++ b/net/xfrm/xfrm_input.c @@ -292,12 +292,15 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) XFRM_SKB_CB(skb)->seq.input.hi = seq_hi; skb_dst_force(skb); + dev_hold(skb->dev); nexthdr = x->type->input(x, skb); if (nexthdr == -EINPROGRESS) return 0; resume: + dev_put(skb->dev); + spin_lock(&x->lock); if (nexthdr <= 0) {
A crash is observed when a decrypted packet is processed in receive path. get_rps_cpus() tries to dereference the skb->dev fields but it appears that the device is freed from the poison pattern. [<ffffffc000af58ec>] get_rps_cpu+0x94/0x2f0 [<ffffffc000af5f94>] netif_rx_internal+0x140/0x1cc [<ffffffc000af6094>] netif_rx+0x74/0x94 [<ffffffc000bc0b6c>] xfrm_input+0x754/0x7d0 [<ffffffc000bc0bf8>] xfrm_input_resume+0x10/0x1c [<ffffffc000ba6eb8>] esp_input_done+0x20/0x30 [<ffffffc0000b64c8>] process_one_work+0x244/0x3fc [<ffffffc0000b7324>] worker_thread+0x2f8/0x418 [<ffffffc0000bb40c>] kthread+0xe0/0xec -013|get_rps_cpu( | dev = 0xFFFFFFC08B688000, | skb = 0xFFFFFFC0C76AAC00 -> ( | dev = 0xFFFFFFC08B688000 -> ( | name = "...................................................... | name_hlist = (next = 0xAAAAAAAAAAAAAAAA, pprev = 0xAAAAAAAAAAA Following are the sequence of events observed - - Encrypted packet in receive path from netdevice is queued - Encrypted packet queued for decryption (asynchronous) - Netdevice brought down and freed - Packet is decrypted and returned through callback in esp_input_done - Packet is queued again for process in network stack using netif_rx Since the device appears to have been freed, the dereference of skb->dev in get_rps_cpus() leads to an unhandled page fault exception. Fix this by holding on to device reference when queueing packets asynchronously and releasing the reference on call back return. v2: Make the change generic to xfrm as mentioned by Steffen and update the title to xfrm Suggested-by: Herbert Xu <herbert@gondor.apana.org.au> Signed-off-by: Jerome Stanislaus <jeromes@codeaurora.org> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org> --- net/xfrm/xfrm_input.c | 3 +++ 1 file changed, 3 insertions(+) if (nexthdr == -EBADMSG) { --