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;