> -----Original Message-----
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> Subject: [PATCH 07/16] esp4: Reorganize esp_output
>
> We need a fallback for ESP at layer 2, so split esp_output into generic
> functions that can be used at layer 3 and layer 2 and use them in
> esp_output. We also add esp_xmit which is used for the layer 2 fallback.
>
> Signed-off-by: Steffen Klassert <steffen.klass...@secunet.com>
> ---
> include/net/esp.h | 16 +++
> net/ipv4/esp4.c | 341 ++++++++++++++++++++++++++-----------------
> -----
> net/ipv4/esp4_offload.c | 102 +++++++++++++++
> 3 files changed, 301 insertions(+), 158 deletions(-)
>
Steffen,
...
In the two places marked below:
> +static int esp_output(struct xfrm_state *x, struct sk_buff *skb) {
> + int alen;
> + int blksize;
> + struct ip_esp_hdr *esph;
> + struct crypto_aead *aead;
> + struct esp_info esp;
> +
> + esp.inplace = true;
> +
> + esp.proto = *skb_mac_header(skb);
> + *skb_mac_header(skb) = IPPROTO_ESP;
> +
> + /* skb is pure payload to encrypt */
> +
> + aead = x->data;
> + alen = crypto_aead_authsize(aead);
> +
> + esp.tfclen = 0;
> + if (x->tfcpad) {
> + struct xfrm_dst *dst = (struct xfrm_dst *)skb_dst(skb);
> + u32 padto;
> +
> + padto = min(x->tfcpad, esp4_get_mtu(x, dst-
> >child_mtu_cached));
> + if (skb->len < padto)
> + esp.tfclen = padto - skb->len;
> + }
> + blksize = ALIGN(crypto_aead_blocksize(aead), 4);
> + esp.clen = ALIGN(skb->len + 2 + esp.tfclen, blksize);
> + esp.plen = esp.clen - skb->len - esp.tfclen;
> + esp.tailen = esp.tfclen + esp.plen + alen;
> +
> + esp.esph = ip_esp_hdr(skb);
> +
> + esp.nfrags = esp_output_head(x, skb, &esp);
> + if (esp.nfrags < 0)
> + return esp.nfrags;
> +
> + esph = esp.esph;
Here
> + esph->spi = x->id.spi;
> +
> + esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.low);
> + esp.seqno = cpu_to_be64(XFRM_SKB_CB(skb)->seq.output.low +
> + ((u64)XFRM_SKB_CB(skb)->seq.output.hi << 32));
> +
> + skb_push(skb, -skb_network_offset(skb));
> +
> + return esp_output_tail(x, skb, &esp);
> +}
>
...
> +static int esp_xmit(struct xfrm_state *x, struct sk_buff *skb,
> +netdev_features_t features) {
> + int err;
> + int alen;
> + int blksize;
> + struct xfrm_offload *xo;
> + struct ip_esp_hdr *esph;
> + struct crypto_aead *aead;
> + struct esp_info esp;
> + bool hw_offload = true;
> +
> + esp.inplace = true;
> +
> + xo = xfrm_offload(skb);
> +
> + if (!xo)
> + return -EINVAL;
> +
> + if (!(features & NETIF_F_HW_ESP) ||
> + (x->xso.offload_handle && x->xso.dev != skb->dev)) {
> + xo->flags |= CRYPTO_FALLBACK;
> + hw_offload = false;
> + }
> +
> + esp.proto = xo->proto;
> +
> + /* skb is pure payload to encrypt */
> +
> + aead = x->data;
> + alen = crypto_aead_authsize(aead);
> +
> + esp.tfclen = 0;
> + /* XXX: Add support for tfc padding here. */
> +
> + blksize = ALIGN(crypto_aead_blocksize(aead), 4);
> + esp.clen = ALIGN(skb->len + 2 + esp.tfclen, blksize);
> + esp.plen = esp.clen - skb->len - esp.tfclen;
> + esp.tailen = esp.tfclen + esp.plen + alen;
> +
> + esp.esph = ip_esp_hdr(skb);
> +
> +
> + if (!hw_offload || (hw_offload && !skb_is_gso(skb))) {
> + esp.nfrags = esp_output_head(x, skb, &esp);
> + if (esp.nfrags < 0)
> + return esp.nfrags;
> + }
> +
> + esph = esp.esph;
And here.
esp_output_head() might do an skb_cow, which then invalidates the esp.esph
pointer and causes a crash later on.
I would expect the ip_esp_hdr() call to be after the esp_output_head() call.
But it seems like this pointer was saved here around the call to
esp_output_head() on purpose.
Is that really so?
Also, esp6/esp6_offload don't make use of esp_info.esph
Only esp_output_tail() uses it, and could have done everything it does without
it.
So maybe it's un-needed?
I am still testing a fix patch for the crash, there may be also something
similar on the RX path, though.
> + esph->spi = x->id.spi;
> +
> + skb_push(skb, -skb_network_offset(skb));
> +
> + if (xo->flags & XFRM_GSO_SEGMENT) {
> + esph->seq_no = htonl(xo->seq.low);
> + } else {
> + ip_hdr(skb)->tot_len = htons(skb->len);
> + ip_send_check(ip_hdr(skb));
> + }
> +
> + if (hw_offload)
> + return 0;
> +
> + esp.seqno = cpu_to_be64(xo->seq.low + ((u64)xo->seq.hi << 32));
> +
> + err = esp_output_tail(x, skb, &esp);
> + if (err < 0)
> + return err;
> +
> + secpath_reset(skb);
> +
> + return 0;
> +}
> +
