Message ID | 2f86c9d7c39cfad21fdb353a183b12651fc5efe9.1583311902.git.lucien.xin@gmail.com |
---|---|
State | Awaiting Upstream |
Delegated to: | David Miller |
Headers | show |
Series | [ipsec] esp: remove the skb from the chain when it's enqueued in cryptd_wq | expand |
On Wed, Mar 04, 2020 at 04:51:42PM +0800, Xin Long wrote: > Xiumei found a panic in esp offload: > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000020 > RIP: 0010:esp_output_done+0x101/0x160 [esp4] > Call Trace: > ? esp_output+0x180/0x180 [esp4] > cryptd_aead_crypt+0x4c/0x90 > cryptd_queue_worker+0x6e/0xa0 > process_one_work+0x1a7/0x3b0 > worker_thread+0x30/0x390 > ? create_worker+0x1a0/0x1a0 > kthread+0x112/0x130 > ? kthread_flush_work_fn+0x10/0x10 > ret_from_fork+0x35/0x40 > > It was caused by that skb secpath is used in esp_output_done() after it's > been released elsewhere. > > The tx path for esp offload is: > > __dev_queue_xmit()-> > validate_xmit_skb_list()-> > validate_xmit_xfrm()-> > esp_xmit()-> > esp_output_tail()-> > aead_request_set_callback(esp_output_done) <--[1] > crypto_aead_encrypt() <--[2] > > In [1], .callback is set, and in [2] it will trigger the worker schedule, > later on a kernel thread will call .callback(esp_output_done), as the call > trace shows. > > But in validate_xmit_xfrm(): > > skb_list_walk_safe(skb, skb2, nskb) { > ... > err = x->type_offload->xmit(x, skb2, esp_features); [esp_xmit] > ... > } > > When the err is -EINPROGRESS, which means this skb2 will be enqueued and > later gets encrypted and sent out by .callback later in a kernel thread, > skb2 should be removed fromt skb chain. Otherwise, it will get processed > again outside validate_xmit_xfrm(), which could release skb secpath, and > cause the panic above. > > This patch is to remove the skb from the chain when it's enqueued in > cryptd_wq. While at it, remove the unnecessary 'if (!skb)' check. > > Fixes: 3dca3f38cfb8 ("xfrm: Separate ESP handling from segmentation for GRO packets.") > Reported-by: Xiumei Mu <xmu@redhat.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> Applied, thanks a lot Xin!
Hi! On 4.3.2020 10.51, Xin Long wrote: > Xiumei found a panic in esp offload: > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000020 > RIP: 0010:esp_output_done+0x101/0x160 [esp4] > Call Trace: > ? esp_output+0x180/0x180 [esp4] > cryptd_aead_crypt+0x4c/0x90 > cryptd_queue_worker+0x6e/0xa0 > process_one_work+0x1a7/0x3b0 > worker_thread+0x30/0x390 > ? create_worker+0x1a0/0x1a0 > kthread+0x112/0x130 > ? kthread_flush_work_fn+0x10/0x10 > ret_from_fork+0x35/0x40 > > It was caused by that skb secpath is used in esp_output_done() after it's > been released elsewhere. > > The tx path for esp offload is: > > __dev_queue_xmit()-> > validate_xmit_skb_list()-> > validate_xmit_xfrm()-> > esp_xmit()-> > esp_output_tail()-> > aead_request_set_callback(esp_output_done) <--[1] > crypto_aead_encrypt() <--[2] > > In [1], .callback is set, and in [2] it will trigger the worker schedule, > later on a kernel thread will call .callback(esp_output_done), as the call > trace shows. > > But in validate_xmit_xfrm(): > > skb_list_walk_safe(skb, skb2, nskb) { > ... > err = x->type_offload->xmit(x, skb2, esp_features); [esp_xmit] > ... > } > > When the err is -EINPROGRESS, which means this skb2 will be enqueued and > later gets encrypted and sent out by .callback later in a kernel thread, > skb2 should be removed fromt skb chain. Otherwise, it will get processed > again outside validate_xmit_xfrm(), which could release skb secpath, and > cause the panic above. > > This patch is to remove the skb from the chain when it's enqueued in > cryptd_wq. While at it, remove the unnecessary 'if (!skb)' check. > > Fixes: 3dca3f38cfb8 ("xfrm: Separate ESP handling from segmentation for GRO packets.") > Reported-by: Xiumei Mu <xmu@redhat.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > net/xfrm/xfrm_device.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c > index 3231ec6..e2db468 100644 > --- a/net/xfrm/xfrm_device.c > +++ b/net/xfrm/xfrm_device.c > @@ -78,8 +78,8 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur > int err; > unsigned long flags; > struct xfrm_state *x; > - struct sk_buff *skb2, *nskb; > struct softnet_data *sd; > + struct sk_buff *skb2, *nskb, *pskb = NULL; > netdev_features_t esp_features = features; > struct xfrm_offload *xo = xfrm_offload(skb); > struct sec_path *sp; > @@ -168,14 +168,14 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur > } else { > if (skb == skb2) > skb = nskb; > - > - if (!skb) > - return NULL; > + else > + pskb->next = nskb; pskb can be NULL on the first round? > continue; > } > > skb_push(skb2, skb2->data - skb_mac_header(skb2)); > + pskb = skb2; > } > > return skb;
On Mon, Mar 9, 2020 at 6:07 PM Mika Penttilä <mika.penttila@nextfour.com> wrote: > > > Hi! > > On 4.3.2020 10.51, Xin Long wrote: > > Xiumei found a panic in esp offload: > > > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000020 > > RIP: 0010:esp_output_done+0x101/0x160 [esp4] > > Call Trace: > > ? esp_output+0x180/0x180 [esp4] > > cryptd_aead_crypt+0x4c/0x90 > > cryptd_queue_worker+0x6e/0xa0 > > process_one_work+0x1a7/0x3b0 > > worker_thread+0x30/0x390 > > ? create_worker+0x1a0/0x1a0 > > kthread+0x112/0x130 > > ? kthread_flush_work_fn+0x10/0x10 > > ret_from_fork+0x35/0x40 > > > > It was caused by that skb secpath is used in esp_output_done() after it's > > been released elsewhere. > > > > The tx path for esp offload is: > > > > __dev_queue_xmit()-> > > validate_xmit_skb_list()-> > > validate_xmit_xfrm()-> > > esp_xmit()-> > > esp_output_tail()-> > > aead_request_set_callback(esp_output_done) <--[1] > > crypto_aead_encrypt() <--[2] > > > > In [1], .callback is set, and in [2] it will trigger the worker schedule, > > later on a kernel thread will call .callback(esp_output_done), as the call > > trace shows. > > > > But in validate_xmit_xfrm(): > > > > skb_list_walk_safe(skb, skb2, nskb) { > > ... > > err = x->type_offload->xmit(x, skb2, esp_features); [esp_xmit] > > ... > > } > > > > When the err is -EINPROGRESS, which means this skb2 will be enqueued and > > later gets encrypted and sent out by .callback later in a kernel thread, > > skb2 should be removed fromt skb chain. Otherwise, it will get processed > > again outside validate_xmit_xfrm(), which could release skb secpath, and > > cause the panic above. > > > > This patch is to remove the skb from the chain when it's enqueued in > > cryptd_wq. While at it, remove the unnecessary 'if (!skb)' check. > > > > Fixes: 3dca3f38cfb8 ("xfrm: Separate ESP handling from segmentation for GRO packets.") > > Reported-by: Xiumei Mu <xmu@redhat.com> > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > net/xfrm/xfrm_device.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c > > index 3231ec6..e2db468 100644 > > --- a/net/xfrm/xfrm_device.c > > +++ b/net/xfrm/xfrm_device.c > > @@ -78,8 +78,8 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur > > int err; > > unsigned long flags; > > struct xfrm_state *x; > > - struct sk_buff *skb2, *nskb; > > struct softnet_data *sd; > > + struct sk_buff *skb2, *nskb, *pskb = NULL; > > netdev_features_t esp_features = features; > > struct xfrm_offload *xo = xfrm_offload(skb); > > struct sec_path *sp; > > @@ -168,14 +168,14 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur > > } else { > > if (skb == skb2) > > skb = nskb; > > - > > - if (!skb) > > - return NULL; > > + else > > + pskb->next = nskb; > > pskb can be NULL on the first round? On the 1st round, skb == skb2. > > > > > continue; > > } > > > > skb_push(skb2, skb2->data - skb_mac_header(skb2)); > > + pskb = skb2; > > } > > > > return skb; >
On 9.3.2020 12.50, Xin Long wrote: > On Mon, Mar 9, 2020 at 6:07 PM Mika Penttilä <mika.penttila@nextfour.com> wrote: >> >> Hi! >> >> On 4.3.2020 10.51, Xin Long wrote: >>> Xiumei found a panic in esp offload: >>> >>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000020 >>> RIP: 0010:esp_output_done+0x101/0x160 [esp4] >>> Call Trace: >>> ? esp_output+0x180/0x180 [esp4] >>> cryptd_aead_crypt+0x4c/0x90 >>> cryptd_queue_worker+0x6e/0xa0 >>> process_one_work+0x1a7/0x3b0 >>> worker_thread+0x30/0x390 >>> ? create_worker+0x1a0/0x1a0 >>> kthread+0x112/0x130 >>> ? kthread_flush_work_fn+0x10/0x10 >>> ret_from_fork+0x35/0x40 >>> >>> It was caused by that skb secpath is used in esp_output_done() after it's >>> been released elsewhere. >>> >>> The tx path for esp offload is: >>> >>> __dev_queue_xmit()-> >>> validate_xmit_skb_list()-> >>> validate_xmit_xfrm()-> >>> esp_xmit()-> >>> esp_output_tail()-> >>> aead_request_set_callback(esp_output_done) <--[1] >>> crypto_aead_encrypt() <--[2] >>> >>> In [1], .callback is set, and in [2] it will trigger the worker schedule, >>> later on a kernel thread will call .callback(esp_output_done), as the call >>> trace shows. >>> >>> But in validate_xmit_xfrm(): >>> >>> skb_list_walk_safe(skb, skb2, nskb) { >>> ... >>> err = x->type_offload->xmit(x, skb2, esp_features); [esp_xmit] >>> ... >>> } >>> >>> When the err is -EINPROGRESS, which means this skb2 will be enqueued and >>> later gets encrypted and sent out by .callback later in a kernel thread, >>> skb2 should be removed fromt skb chain. Otherwise, it will get processed >>> again outside validate_xmit_xfrm(), which could release skb secpath, and >>> cause the panic above. >>> >>> This patch is to remove the skb from the chain when it's enqueued in >>> cryptd_wq. While at it, remove the unnecessary 'if (!skb)' check. >>> >>> Fixes: 3dca3f38cfb8 ("xfrm: Separate ESP handling from segmentation for GRO packets.") >>> Reported-by: Xiumei Mu <xmu@redhat.com> >>> Signed-off-by: Xin Long <lucien.xin@gmail.com> >>> --- >>> net/xfrm/xfrm_device.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c >>> index 3231ec6..e2db468 100644 >>> --- a/net/xfrm/xfrm_device.c >>> +++ b/net/xfrm/xfrm_device.c >>> @@ -78,8 +78,8 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur >>> int err; >>> unsigned long flags; >>> struct xfrm_state *x; >>> - struct sk_buff *skb2, *nskb; >>> struct softnet_data *sd; >>> + struct sk_buff *skb2, *nskb, *pskb = NULL; >>> netdev_features_t esp_features = features; >>> struct xfrm_offload *xo = xfrm_offload(skb); >>> struct sec_path *sp; >>> @@ -168,14 +168,14 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur >>> } else { >>> if (skb == skb2) >>> skb = nskb; >>> - >>> - if (!skb) >>> - return NULL; >>> + else >>> + pskb->next = nskb; >> pskb can be NULL on the first round? > On the 1st round, skb == skb2. Yes, I misread the patch, thanks. > >> >> >>> continue; >>> } >>> >>> skb_push(skb2, skb2->data - skb_mac_header(skb2)); >>> + pskb = skb2; >>> } >>> >>> return skb;
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c index 3231ec6..e2db468 100644 --- a/net/xfrm/xfrm_device.c +++ b/net/xfrm/xfrm_device.c @@ -78,8 +78,8 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur int err; unsigned long flags; struct xfrm_state *x; - struct sk_buff *skb2, *nskb; struct softnet_data *sd; + struct sk_buff *skb2, *nskb, *pskb = NULL; netdev_features_t esp_features = features; struct xfrm_offload *xo = xfrm_offload(skb); struct sec_path *sp; @@ -168,14 +168,14 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur } else { if (skb == skb2) skb = nskb; - - if (!skb) - return NULL; + else + pskb->next = nskb; continue; } skb_push(skb2, skb2->data - skb_mac_header(skb2)); + pskb = skb2; } return skb;
Xiumei found a panic in esp offload: BUG: unable to handle kernel NULL pointer dereference at 0000000000000020 RIP: 0010:esp_output_done+0x101/0x160 [esp4] Call Trace: ? esp_output+0x180/0x180 [esp4] cryptd_aead_crypt+0x4c/0x90 cryptd_queue_worker+0x6e/0xa0 process_one_work+0x1a7/0x3b0 worker_thread+0x30/0x390 ? create_worker+0x1a0/0x1a0 kthread+0x112/0x130 ? kthread_flush_work_fn+0x10/0x10 ret_from_fork+0x35/0x40 It was caused by that skb secpath is used in esp_output_done() after it's been released elsewhere. The tx path for esp offload is: __dev_queue_xmit()-> validate_xmit_skb_list()-> validate_xmit_xfrm()-> esp_xmit()-> esp_output_tail()-> aead_request_set_callback(esp_output_done) <--[1] crypto_aead_encrypt() <--[2] In [1], .callback is set, and in [2] it will trigger the worker schedule, later on a kernel thread will call .callback(esp_output_done), as the call trace shows. But in validate_xmit_xfrm(): skb_list_walk_safe(skb, skb2, nskb) { ... err = x->type_offload->xmit(x, skb2, esp_features); [esp_xmit] ... } When the err is -EINPROGRESS, which means this skb2 will be enqueued and later gets encrypted and sent out by .callback later in a kernel thread, skb2 should be removed fromt skb chain. Otherwise, it will get processed again outside validate_xmit_xfrm(), which could release skb secpath, and cause the panic above. This patch is to remove the skb from the chain when it's enqueued in cryptd_wq. While at it, remove the unnecessary 'if (!skb)' check. Fixes: 3dca3f38cfb8 ("xfrm: Separate ESP handling from segmentation for GRO packets.") Reported-by: Xiumei Mu <xmu@redhat.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/xfrm/xfrm_device.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)