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;