Re: [PATCH net] ipv6: rpl: unclone the skb before modifying the packet

[Andrea Mayer <andrea.mayer@uniroma2.it>]

On Sat,  3 Oct 2026 19:30:58 +0000
Kuniyuki Iwashima <kuniyu@google.com> wrote:

Hi Kuniyuki,

Thanks for the review and for the diff.

> [snip]
> 
> > +	if (skb_cloned(skb)) {
> > +		if (pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
> 
> ipv6_rpl_srh_rcv() already has pskb_expand_head() call later
> and I think this can be merged there.
> 
> [snip]
> 
> >  	hdr->segments_left--;
> >  	i = n - hdr->segments_left;
> 
> This and the later swap(ipv6_hdr(skb)->daddr, ohdr->rpl_segaddr[i])
> only modify the header before the existing pskb_expand_head().

Agreed, the unclone can be merged with the existing pskb_expand_head()
once the writes are moved after it.

> Also this seems wrong because skb_postpull_rcsum() is applied to
> the modified header, which should corrupt checksum.

Yes, this is a separate bug. v1 was meant to cover only the clone for
now. I will fix it as well.

> I think we can merge the clone check and pskb_expand_head() with
> skb_cow() and tmp IPv6 buffer like this.
> 
> compiled only:
> 
> ---8<---
> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> index 09a4552f7f08..af285edd4d16 100644
> --- a/net/ipv6/exthdrs.c
> +++ b/net/ipv6/exthdrs.c
> @@ -485,6 +485,7 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb, struct inet6_dev *idev)
>  	struct net *net = dev_net(skb->dev);
>  	struct ipv6hdr *oldhdr;
>  	unsigned int chdr_len;
> +	struct in6_addr addr;
>  	unsigned char *buf;
>  	int accept_rpl_seg;
>  	int i, err;
> @@ -548,9 +549,6 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb, struct inet6_dev *idev)
>  		return -1;
>  	}
>  
> -	hdr->segments_left--;
> -	i = n - hdr->segments_left;
> -
>  	buf = kcalloc(struct_size(hdr, segments.addr, n + 2), 2, GFP_ATOMIC);
>  	if (unlikely(!buf)) {
>  		kfree_skb(skb);
> @@ -559,6 +557,8 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb, struct inet6_dev *idev)
>  
>  	ohdr = (struct ipv6_rpl_sr_hdr *)buf;
>  	ipv6_rpl_srh_decompress(ohdr, hdr, &ipv6_hdr(skb)->daddr, n);
> +	ohdr->segments_left--;
> +	i = n - ohdr->segments_left;
>  	chdr = (struct ipv6_rpl_sr_hdr *)(buf + ((ohdr->hdrlen + 1) << 3));
>  
>  	if (ipv6_addr_is_multicast(&ohdr->rpl_segaddr[i])) {
> @@ -575,28 +575,24 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb, struct inet6_dev *idev)
>  		return -1;
>  	}
>  
> -	swap(ipv6_hdr(skb)->daddr, ohdr->rpl_segaddr[i]);
> +	addr = ohdr->rpl_segaddr[i];
> +	ohdr->rpl_segaddr[i] = ipv6_hdr(skb)->daddr;
>  
> -	ipv6_rpl_srh_compress(chdr, ohdr, &ipv6_hdr(skb)->daddr, n);
> -
> -	oldhdr = ipv6_hdr(skb);
> +	ipv6_rpl_srh_compress(chdr, ohdr, &addr, n);
>  
>  	skb_pull(skb, ((hdr->hdrlen + 1) << 3));
> -	skb_postpull_rcsum(skb, oldhdr,
> +	skb_postpull_rcsum(skb, ipv6_hdr(skb),
>  			   sizeof(struct ipv6hdr) + ((hdr->hdrlen + 1) << 3));
>  	chdr_len = sizeof(struct ipv6hdr) + ((chdr->hdrlen + 1) << 3);
> -	if (unlikely(!hdr->segments_left ||
> -		     skb_headroom(skb) < chdr_len + skb->mac_len)) {
> -		if (pskb_expand_head(skb, chdr_len + skb->mac_len, 0,
> -				     GFP_ATOMIC)) {
> -			__IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)), IPSTATS_MIB_OUTDISCARDS);
> -			kfree_skb(skb);
> -			kfree(buf);
> -			return -1;
> -		}
> -
> -		oldhdr = ipv6_hdr(skb);
> +	if (unlikely(skb_cow(skb, chdr_len + skb->mac_len))) {
> +		__IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)), IPSTATS_MIB_OUTDISCARDS);
> +		kfree_skb_reason(skb, SKB_DROP_REASON_NOMEM);
> +		kfree(buf);
> +		return -1;
>  	}
> +
> +	oldhdr = ipv6_hdr(skb);
> +
>  	skb_push(skb, chdr_len);
>  	skb_reset_network_header(skb);
>  	skb_mac_header_rebuild(skb);
> @@ -605,6 +601,7 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb, struct inet6_dev *idev)
>  	memmove(ipv6_hdr(skb), oldhdr, sizeof(struct ipv6hdr));
>  	memcpy(skb_transport_header(skb), chdr, (chdr->hdrlen + 1) << 3);
>  
> +	ipv6_hdr(skb)->daddr = addr;
>  	ipv6_hdr(skb)->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
>  	skb_postpush_rcsum(skb, ipv6_hdr(skb),
>  			   sizeof(struct ipv6hdr) + ((chdr->hdrlen + 1) << 3));
> ---8<---

I will take your diff into account for v2.

Thanks,
Andrea

pw-bot: changes-requested