From: "Mika Penttilä" <mika.penttila@nextfour.com>
To: Xin Long <lucien.xin@gmail.com>
Cc: network dev <netdev@vger.kernel.org>,
Steffen Klassert <steffen.klassert@secunet.com>,
Herbert Xu <herbert@gondor.apana.org.au>,
"David S. Miller" <davem@davemloft.net>,
Sabrina Dubroca <sd@queasysnail.net>
Subject: Re: [PATCH ipsec] esp: remove the skb from the chain when it's enqueued in cryptd_wq
Date: Mon, 9 Mar 2020 13:06:20 +0200 [thread overview]
Message-ID: <bb90ae9e-9d8f-e860-5d82-dbd06081905c@nextfour.com> (raw)
In-Reply-To: <CADvbK_dqRXr3D1WgLDXqiBhpyw+QGRrvwugqDhOMr_kpQVi3QA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3683 bytes --]
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;
[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 3157 bytes --]
prev parent reply other threads:[~2020-03-09 11:06 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-04 8:51 [PATCH ipsec] esp: remove the skb from the chain when it's enqueued in cryptd_wq Xin Long
2020-03-09 9:15 ` Steffen Klassert
2020-03-09 10:07 ` Mika Penttilä
2020-03-09 10:50 ` Xin Long
2020-03-09 11:06 ` Mika Penttilä [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bb90ae9e-9d8f-e860-5d82-dbd06081905c@nextfour.com \
--to=mika.penttila@nextfour.com \
--cc=davem@davemloft.net \
--cc=herbert@gondor.apana.org.au \
--cc=lucien.xin@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=sd@queasysnail.net \
--cc=steffen.klassert@secunet.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).