Re: [PATCH net v2 1/2] ipv4: recompile IP options on a private copy in ipv4_send_dest_unreach()

[Paolo Abeni <pabeni@redhat.com>]

On 11/12/26 14:46, Ido Schimmel wrote:
> On Mon, Nov 09, 2026 at 02:17:35PM +0100, Eric Dumazet wrote:
>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>> index 3a90c8d28b940bf615c165d9b30ac634943586e1..9af72cb892034deff2bb3169911c02c025d73cf2 100644
>> --- a/net/ipv4/route.c
>> +++ b/net/ipv4/route.c
>> @@ -1256,6 +1256,7 @@ EXPORT_INDIRECT_CALLABLE(ipv4_dst_check);
>>   
>>   static void ipv4_send_dest_unreach(struct sk_buff *skb)
>>   {
>> +	struct sk_buff *nskb = NULL;
>>   	struct inet_skb_parm parm;
>>   	struct net_device *dev;
>>   	int res;
>> @@ -1271,6 +1272,16 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
>>   	if (ip_hdr(skb)->ihl > 5) {
>>   		if (!pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4))
>>   			return;
>> +
>> +		/* __ip_options_compile() writes into the options, and
>> +		 * callers of dst_link_failure() may cache pointers into
>> +		 * skb->head: work on a private copy.
>> +		 */
>> +		nskb = pskb_copy(skb, GFP_ATOMIC);
>> +		if (!nskb)
>> +			return;
>> +		skb = nskb;
>> +
>>   		parm.opt.optlen = ip_hdr(skb)->ihl * 4 - sizeof(struct iphdr);
>>   
>>   		rcu_read_lock();
>> @@ -1279,9 +1290,11 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
>>   		rcu_read_unlock();
>>   
>>   		if (res)
>> -			return;
>> +			goto out;
>>   	}
>>   	__icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, 0, &parm);
>> +out:
>> +	consume_skb(nskb);
>>   }
> 
> Not sure that what Sashiko describes can happen today, but I wouldn't
> rely on the network header not being in the headroom. Something like (on
> top of your patch):
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 630143ca0f31..77c03620f89d 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1282,6 +1282,10 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
>   		if (!pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4))
>   			return;
>   
> +		/* pskb_copy() does not copy the headroom */
> +		if (skb_network_offset(skb) < 0)
> +			return;
> +
>   		/* __ip_options_compile() writes into the options, and
>   		 * callers of dst_link_failure() may cache pointers into
>   		 * skb->head: work on a private copy.

I think the above makes sense.

/P