Re: [PATCH IPV6 v3 1/1] ipv6: allocate enough headroom in ip6_finish_output2()

From: Vasily Averin
Date: Tue Jul 13 2021 - 03:46:24 EST


I've found 2 problems in this patch,
and I'm going to resend new patch version soon.

On 7/12/21 9:45 AM, Vasily Averin wrote:
> index ff4f9eb..0efcb9b 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -60,10 +60,38 @@ static int ip6_finish_output2(struct net *net, struct sock *sk, struct sk_buff *
> {
> struct dst_entry *dst = skb_dst(skb);
> struct net_device *dev = dst->dev;
> + unsigned int hh_len = LL_RESERVED_SPACE(dev);
> + int delta = hh_len - skb_headroom(skb);
> const struct in6_addr *nexthop;
> struct neighbour *neigh;
> int ret;
>
> + /* Be paranoid, rather than too clever. */
> + if (unlikely(delta > 0) && dev->header_ops) {
> + /* pskb_expand_head() might crash, if skb is shared */
> + if (skb_shared(skb)) {
> + struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
> +
> + if (likely(nskb)) {
> + if (skb->sk)
> + skb_set_owner_w(skb, skb->sk);

need to assign sk not to skb but to nskb

> + consume_skb(skb);
> + } else {
> + kfree_skb(skb);

It is quite strange to call consume_skb() on one case and kfree_skb() in another one.
We know that original skb was shared so we should not call kfree_skb here.

Btw I've noticed similar problem in few other cases:
in pptp_xmit, pvc_xmit, ip_vs_prepare_tunneled_skb
they call consume_skb() in case of success and kfree_skb on error path.
It looks like potential bug for me.

> + }
> + skb = nskb;
> + }
> + if (skb &&
> + pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC)) {
> + kfree_skb(skb);
> + skb = NULL;
> + }
> + if (!skb) {
> + IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTDISCARDS);
> + return -ENOMEM;
> + }
> + }
> +
> if (ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr)) {
> struct inet6_dev *idev = ip6_dst_idev(skb_dst(skb));
>
>