[PATCH net] net: openvswitch: validate transport header presence in set_ipv6_addr

[PATCH net] net: openvswitch: validate transport header presence in set_ipv6_addr

[Fernando Fernandez Mancera]

When executing IPv6 address rewrite actions on IPv6 fragments,
set_ipv6_addr() might attempt to recalculate the L4 header checksum
calling skb_transport_header() with an uninitialized transport header.
This can potentially lead to a OOB write. A warning is triggered with
DEBUG_NET=y.

Fix this by recalculating the checksum only if there is a valid
transport header in the skb. Otherwise, assume this is a fragment and
skip it.

See the syzbot trace:

!skb_transport_header_was_set(skb)
WARNING: ./include/linux/skbuff.h:3075 at skb_transport_header include/linux/skbuff.h:3075 [inline], CPU#1: syz-executor463/5635
WARNING: ./include/linux/skbuff.h:3075 at skb_transport_offset include/linux/skbuff.h:3250 [inline], CPU#1: syz-executor463/5635
WARNING: ./include/linux/skbuff.h:3075 at update_ipv6_checksum net/openvswitch/actions.c:361 [inline], CPU#1: syz-executor463/5635
WARNING: ./include/linux/skbuff.h:3075 at set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399, CPU#1: syz-executor463/5635
[...]
RIP: 0010:skb_transport_header include/linux/skbuff.h:3075 [inline]
RIP: 0010:skb_transport_offset include/linux/skbuff.h:3250 [inline]
RIP: 0010:update_ipv6_checksum net/openvswitch/actions.c:361 [inline]
RIP: 0010:set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399
[...]
Call Trace:
 <TASK>
 set_ipv6 net/openvswitch/actions.c:531 [inline]
 do_execute_actions+0x557e/0x8600 net/openvswitch/actions.c:1366
 ovs_execute_actions+0xde/0x520 net/openvswitch/actions.c:1592
 ovs_packet_cmd_execute+0xb4f/0xf10 net/openvswitch/datapath.c:705
 genl_family_rcv_msg_doit+0x233/0x340 net/netlink/genetlink.c:1114
 genl_rcv_msg+0x614/0x7a0 net/netlink/genetlink.c:1209
 netlink_rcv_skb+0x226/0x4a0 net/netlink/af_netlink.c:2572
 genl_rcv+0x28/0x40 net/netlink/genetlink.c:1218
 netlink_unicast+0x7bd/0x940 net/netlink/af_netlink.c:1361
 netlink_sendmsg+0x813/0xb40 net/netlink/af_netlink.c:1916
 sock_sendmsg_nosec+0x14e/0x190 net/socket.c:800

Reported-by: syzbot+4cc63fcfb3845e149969@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=4cc63fcfb3845e149969
Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
 net/openvswitch/actions.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index dc5ff859f114..a14668565ec4 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -395,7 +395,7 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
 			  __be32 addr[4], const __be32 new_addr[4],
 			  bool recalculate_csum)
 {
-	if (recalculate_csum)
+	if (recalculate_csum && skb_transport_header_was_set(skb))
 		update_ipv6_checksum(skb, l4_proto, addr, new_addr);
 
 	skb_clear_hash(skb);
-- 
2.55.0

Re: [PATCH net] net: openvswitch: validate transport header presence in set_ipv6_addr

[Ilya Maximets]

On 10/1/26 10:52 AM, Fernando Fernandez Mancera wrote:
> When executing IPv6 address rewrite actions on IPv6 fragments,
> set_ipv6_addr() might attempt to recalculate the L4 header checksum
> calling skb_transport_header() with an uninitialized transport header.
> This can potentially lead to a OOB write.

Not really.  later fragments have l4_proto set to NEXTHDR_FRAGMENT
and so no writes will be performed.  See parse_ipv6hdr().

> A warning is triggered with DEBUG_NET=y.

Yeah, while the value is not actually used to access the packet data,
the skbuff core generates a warning on read.

> 
> Fix this by recalculating the checksum only if there is a valid
> transport header in the skb. Otherwise, assume this is a fragment and
> skip it.
> 
> See the syzbot trace:
> 
> !skb_transport_header_was_set(skb)
> WARNING: ./include/linux/skbuff.h:3075 at skb_transport_header include/linux/skbuff.h:3075 [inline], CPU#1: syz-executor463/5635
> WARNING: ./include/linux/skbuff.h:3075 at skb_transport_offset include/linux/skbuff.h:3250 [inline], CPU#1: syz-executor463/5635
> WARNING: ./include/linux/skbuff.h:3075 at update_ipv6_checksum net/openvswitch/actions.c:361 [inline], CPU#1: syz-executor463/5635
> WARNING: ./include/linux/skbuff.h:3075 at set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399, CPU#1: syz-executor463/5635
> [...]
> RIP: 0010:skb_transport_header include/linux/skbuff.h:3075 [inline]
> RIP: 0010:skb_transport_offset include/linux/skbuff.h:3250 [inline]
> RIP: 0010:update_ipv6_checksum net/openvswitch/actions.c:361 [inline]
> RIP: 0010:set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399
> [...]
> Call Trace:
>  <TASK>
>  set_ipv6 net/openvswitch/actions.c:531 [inline]
>  do_execute_actions+0x557e/0x8600 net/openvswitch/actions.c:1366
>  ovs_execute_actions+0xde/0x520 net/openvswitch/actions.c:1592
>  ovs_packet_cmd_execute+0xb4f/0xf10 net/openvswitch/datapath.c:705
>  genl_family_rcv_msg_doit+0x233/0x340 net/netlink/genetlink.c:1114
>  genl_rcv_msg+0x614/0x7a0 net/netlink/genetlink.c:1209
>  netlink_rcv_skb+0x226/0x4a0 net/netlink/af_netlink.c:2572
>  genl_rcv+0x28/0x40 net/netlink/genetlink.c:1218
>  netlink_unicast+0x7bd/0x940 net/netlink/af_netlink.c:1361
>  netlink_sendmsg+0x813/0xb40 net/netlink/af_netlink.c:1916
>  sock_sendmsg_nosec+0x14e/0x190 net/socket.c:800
> 
> Reported-by: syzbot+4cc63fcfb3845e149969@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=4cc63fcfb3845e149969
> Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> ---
>  net/openvswitch/actions.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index dc5ff859f114..a14668565ec4 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -395,7 +395,7 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
>  			  __be32 addr[4], const __be32 new_addr[4],
>  			  bool recalculate_csum)
>  {
> -	if (recalculate_csum)
> +	if (recalculate_csum && skb_transport_header_was_set(skb))
>  		update_ipv6_checksum(skb, l4_proto, addr, new_addr);
I'd suggest moving the check into update_ipv6_checksum() and maybe add
a small comment that the check is only to avoid warning on reading the
offset that wasn't set, offset must always be set for any l4_proto for
which the checksum is actually getting updated.

Best regards, Ilya Maximets.

Re: [PATCH net] net: openvswitch: validate transport header presence in set_ipv6_addr

[Fernando Fernandez Mancera]

On 10/1/26 1:15 PM, Ilya Maximets wrote:
> On 10/1/26 10:52 AM, Fernando Fernandez Mancera wrote:
>> When executing IPv6 address rewrite actions on IPv6 fragments,
>> set_ipv6_addr() might attempt to recalculate the L4 header checksum
>> calling skb_transport_header() with an uninitialized transport header.
>> This can potentially lead to a OOB write.
> 
> Not really.  later fragments have l4_proto set to NEXTHDR_FRAGMENT
> and so no writes will be performed.  See parse_ipv6hdr().
> 

Oh, I didn't notice that. In that case, this is harmless. I will update 
the commit message, thanks!

>> A warning is triggered with DEBUG_NET=y.
> 
> Yeah, while the value is not actually used to access the packet data,
> the skbuff core generates a warning on read.
> 
>>
>> Fix this by recalculating the checksum only if there is a valid
>> transport header in the skb. Otherwise, assume this is a fragment and
>> skip it.
>>
>> See the syzbot trace:
>>
>> !skb_transport_header_was_set(skb)
>> WARNING: ./include/linux/skbuff.h:3075 at skb_transport_header include/linux/skbuff.h:3075 [inline], CPU#1: syz-executor463/5635
>> WARNING: ./include/linux/skbuff.h:3075 at skb_transport_offset include/linux/skbuff.h:3250 [inline], CPU#1: syz-executor463/5635
>> WARNING: ./include/linux/skbuff.h:3075 at update_ipv6_checksum net/openvswitch/actions.c:361 [inline], CPU#1: syz-executor463/5635
>> WARNING: ./include/linux/skbuff.h:3075 at set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399, CPU#1: syz-executor463/5635
>> [...]
>> RIP: 0010:skb_transport_header include/linux/skbuff.h:3075 [inline]
>> RIP: 0010:skb_transport_offset include/linux/skbuff.h:3250 [inline]
>> RIP: 0010:update_ipv6_checksum net/openvswitch/actions.c:361 [inline]
>> RIP: 0010:set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399
>> [...]
>> Call Trace:
>>   <TASK>
>>   set_ipv6 net/openvswitch/actions.c:531 [inline]
>>   do_execute_actions+0x557e/0x8600 net/openvswitch/actions.c:1366
>>   ovs_execute_actions+0xde/0x520 net/openvswitch/actions.c:1592
>>   ovs_packet_cmd_execute+0xb4f/0xf10 net/openvswitch/datapath.c:705
>>   genl_family_rcv_msg_doit+0x233/0x340 net/netlink/genetlink.c:1114
>>   genl_rcv_msg+0x614/0x7a0 net/netlink/genetlink.c:1209
>>   netlink_rcv_skb+0x226/0x4a0 net/netlink/af_netlink.c:2572
>>   genl_rcv+0x28/0x40 net/netlink/genetlink.c:1218
>>   netlink_unicast+0x7bd/0x940 net/netlink/af_netlink.c:1361
>>   netlink_sendmsg+0x813/0xb40 net/netlink/af_netlink.c:1916
>>   sock_sendmsg_nosec+0x14e/0x190 net/socket.c:800
>>
>> Reported-by: syzbot+4cc63fcfb3845e149969@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=4cc63fcfb3845e149969
>> Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
>> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
>> ---
>>   net/openvswitch/actions.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index dc5ff859f114..a14668565ec4 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -395,7 +395,7 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
>>   			  __be32 addr[4], const __be32 new_addr[4],
>>   			  bool recalculate_csum)
>>   {
>> -	if (recalculate_csum)
>> +	if (recalculate_csum && skb_transport_header_was_set(skb))
>>   		update_ipv6_checksum(skb, l4_proto, addr, new_addr);
> I'd suggest moving the check into update_ipv6_checksum() and maybe add
> a small comment that the check is only to avoid warning on reading the
> offset that wasn't set, offset must always be set for any l4_proto for
> which the checksum is actually getting updated.
> 

Will do that. Thanks Ilya.

> Best regards, Ilya Maximets.

Re: [ovs-dev] [PATCH net] net: openvswitch: validate transport header presence in set_ipv6_addr

[Ilya Maximets]

On 10/1/26 1:20 PM, Fernando Fernandez Mancera via dev wrote:
> On 10/1/26 1:15 PM, Ilya Maximets wrote:
>> On 10/1/26 10:52 AM, Fernando Fernandez Mancera wrote:
>>> When executing IPv6 address rewrite actions on IPv6 fragments,
>>> set_ipv6_addr() might attempt to recalculate the L4 header checksum
>>> calling skb_transport_header() with an uninitialized transport header.
>>> This can potentially lead to a OOB write.
>>
>> Not really.  later fragments have l4_proto set to NEXTHDR_FRAGMENT
>> and so no writes will be performed.  See parse_ipv6hdr().
>>
> 
> Oh, I didn't notice that. In that case, this is harmless. I will update 
> the commit message, thanks!
> 

[...]

>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>> index dc5ff859f114..a14668565ec4 100644
>>> --- a/net/openvswitch/actions.c
>>> +++ b/net/openvswitch/actions.c
>>> @@ -395,7 +395,7 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
>>>   			  __be32 addr[4], const __be32 new_addr[4],
>>>   			  bool recalculate_csum)
>>>   {
>>> -	if (recalculate_csum)
>>> +	if (recalculate_csum && skb_transport_header_was_set(skb))
>>>   		update_ipv6_checksum(skb, l4_proto, addr, new_addr);
>> I'd suggest moving the check into update_ipv6_checksum() and maybe add
>> a small comment that the check is only to avoid warning on reading the
>> offset that wasn't set, offset must always be set for any l4_proto for
>> which the checksum is actually getting updated.
>>
> 
> Will do that. Thanks Ilya.
Actually, it might be better to check for NEXTHDR_FRAGMENT and return
early only in this case.  This way if we ever have the offset missing
for any l4_ptoto that needs it, we'll still get a warning instead of
silently not updating the checksum.  This will be close to what the
update_ip_l4_checksum() is doing.

At the same time, update_ip_l4_checksum() is able to get the transport
offset without a warning, so another alternative is to actually set the
transport offset in parse_ipv6hdr() to something like frag_off, which
would closer match the behavior of ipv4 where check_iphdr() sets the
transport header unconditionally to the next byte after the network header.
This is fine because transport headers are never accessed when they are
not set in the key.  The network checksum update cases are the only ones
where the actions are even allowed to execute and touch higher level
headers that may not be present in the packet.  At the same time the
frag_off is kind of an arbitrary value, so I'm not sure.

WDYT?

Eelco, Aaron, do you maybe have a preference?

Best regards, Ilya Maximets.

Re: [ovs-dev] [PATCH net] net: openvswitch: validate transport header presence in set_ipv6_addr

[Fernando Fernandez Mancera]

On 10/1/26 2:00 PM, Ilya Maximets wrote:
> On 10/1/26 1:20 PM, Fernando Fernandez Mancera via dev wrote:
>> On 10/1/26 1:15 PM, Ilya Maximets wrote:
>>> On 10/1/26 10:52 AM, Fernando Fernandez Mancera wrote:
>>>> When executing IPv6 address rewrite actions on IPv6 fragments,
>>>> set_ipv6_addr() might attempt to recalculate the L4 header checksum
>>>> calling skb_transport_header() with an uninitialized transport header.
>>>> This can potentially lead to a OOB write.
>>>
>>> Not really.  later fragments have l4_proto set to NEXTHDR_FRAGMENT
>>> and so no writes will be performed.  See parse_ipv6hdr().
>>>
>>
>> Oh, I didn't notice that. In that case, this is harmless. I will update
>> the commit message, thanks!
>>
> 
> [...]
> 
>>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>>> index dc5ff859f114..a14668565ec4 100644
>>>> --- a/net/openvswitch/actions.c
>>>> +++ b/net/openvswitch/actions.c
>>>> @@ -395,7 +395,7 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
>>>>    			  __be32 addr[4], const __be32 new_addr[4],
>>>>    			  bool recalculate_csum)
>>>>    {
>>>> -	if (recalculate_csum)
>>>> +	if (recalculate_csum && skb_transport_header_was_set(skb))
>>>>    		update_ipv6_checksum(skb, l4_proto, addr, new_addr);
>>> I'd suggest moving the check into update_ipv6_checksum() and maybe add
>>> a small comment that the check is only to avoid warning on reading the
>>> offset that wasn't set, offset must always be set for any l4_proto for
>>> which the checksum is actually getting updated.
>>>
>>
>> Will do that. Thanks Ilya.
> Actually, it might be better to check for NEXTHDR_FRAGMENT and return
> early only in this case.  This way if we ever have the offset missing
> for any l4_ptoto that needs it, we'll still get a warning instead of
> silently not updating the checksum.  This will be close to what the
> update_ip_l4_checksum() is doing.
> 
> At the same time, update_ip_l4_checksum() is able to get the transport
> offset without a warning, so another alternative is to actually set the
> transport offset in parse_ipv6hdr() to something like frag_off, which
> would closer match the behavior of ipv4 where check_iphdr() sets the
> transport header unconditionally to the next byte after the network header.
> This is fine because transport headers are never accessed when they are
> not set in the key.  The network checksum update cases are the only ones
> where the actions are even allowed to execute and touch higher level
> headers that may not be present in the packet.  At the same time the
> frag_off is kind of an arbitrary value, so I'm not sure.
> 
> WDYT?

I noticed this but IMHO it is a worse pattern. In the sense that one 
persone in the future might use the transport header without noticing 
this and the warning won't show up because the transport header will be 
set incorrectly. This could cause an actual OOB.

I will trust your taste in this matter since I am not that familiar with 
the code.

Thanks,
Fernando.

> Eelco, Aaron, do you maybe have a preference?
> 
> Best regards, Ilya Maximets.

Re: [ovs-dev] [PATCH net] net: openvswitch: validate transport header presence in set_ipv6_addr

[Ilya Maximets]

On 10/1/26 3:29 PM, Fernando Fernandez Mancera wrote:
> On 10/1/26 2:00 PM, Ilya Maximets wrote:
>> On 10/1/26 1:20 PM, Fernando Fernandez Mancera via dev wrote:
>>> On 10/1/26 1:15 PM, Ilya Maximets wrote:
>>>> On 10/1/26 10:52 AM, Fernando Fernandez Mancera wrote:
>>>>> When executing IPv6 address rewrite actions on IPv6 fragments,
>>>>> set_ipv6_addr() might attempt to recalculate the L4 header checksum
>>>>> calling skb_transport_header() with an uninitialized transport header.
>>>>> This can potentially lead to a OOB write.
>>>>
>>>> Not really.  later fragments have l4_proto set to NEXTHDR_FRAGMENT
>>>> and so no writes will be performed.  See parse_ipv6hdr().
>>>>
>>>
>>> Oh, I didn't notice that. In that case, this is harmless. I will update
>>> the commit message, thanks!
>>>
>>
>> [...]
>>
>>>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>>>> index dc5ff859f114..a14668565ec4 100644
>>>>> --- a/net/openvswitch/actions.c
>>>>> +++ b/net/openvswitch/actions.c
>>>>> @@ -395,7 +395,7 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
>>>>>    			  __be32 addr[4], const __be32 new_addr[4],
>>>>>    			  bool recalculate_csum)
>>>>>    {
>>>>> -	if (recalculate_csum)
>>>>> +	if (recalculate_csum && skb_transport_header_was_set(skb))
>>>>>    		update_ipv6_checksum(skb, l4_proto, addr, new_addr);
>>>> I'd suggest moving the check into update_ipv6_checksum() and maybe add
>>>> a small comment that the check is only to avoid warning on reading the
>>>> offset that wasn't set, offset must always be set for any l4_proto for
>>>> which the checksum is actually getting updated.
>>>>
>>>
>>> Will do that. Thanks Ilya.
>> Actually, it might be better to check for NEXTHDR_FRAGMENT and return
>> early only in this case.  This way if we ever have the offset missing
>> for any l4_ptoto that needs it, we'll still get a warning instead of
>> silently not updating the checksum.  This will be close to what the
>> update_ip_l4_checksum() is doing.
>>
>> At the same time, update_ip_l4_checksum() is able to get the transport
>> offset without a warning, so another alternative is to actually set the
>> transport offset in parse_ipv6hdr() to something like frag_off, which
>> would closer match the behavior of ipv4 where check_iphdr() sets the
>> transport header unconditionally to the next byte after the network header.
>> This is fine because transport headers are never accessed when they are
>> not set in the key.  The network checksum update cases are the only ones
>> where the actions are even allowed to execute and touch higher level
>> headers that may not be present in the packet.  At the same time the
>> frag_off is kind of an arbitrary value, so I'm not sure.
>>
>> WDYT?
> 
> I noticed this but IMHO it is a worse pattern. In the sense that one 
> persone in the future might use the transport header without noticing 
> this and the warning won't show up because the transport header will be 
> set incorrectly. This could cause an actual OOB.

Yeah, that's true.  It's better to keep the transport header unset if
there is actually no transport header.  We may also adjust the ipv4 path
to avoid having it set for later fragments, but that's unrelated for the
issue at hand.

> I will trust your taste in this matter since I am not that familiar with 
> the code.

Let's just go with the check inside update_ipv6_checksum() before reading
the value.  Seems like the best option.

Best regards, Ilya Maximets.

> 
> Thanks,
> Fernando.
> 
>> Eelco, Aaron, do you maybe have a preference?
>>
>> Best regards, Ilya Maximets.
> 

Re: [ovs-dev] [PATCH net] net: openvswitch: validate transport header presence in set_ipv6_addr

[Fernando Fernandez Mancera]

On 10/2/26 12:10 AM, Ilya Maximets wrote:
> On 10/1/26 3:29 PM, Fernando Fernandez Mancera wrote:
>> On 10/1/26 2:00 PM, Ilya Maximets wrote:
>>> On 10/1/26 1:20 PM, Fernando Fernandez Mancera via dev wrote:
>>>> On 10/1/26 1:15 PM, Ilya Maximets wrote:
>>>>> On 10/1/26 10:52 AM, Fernando Fernandez Mancera wrote:
>>>>>> When executing IPv6 address rewrite actions on IPv6 fragments,
>>>>>> set_ipv6_addr() might attempt to recalculate the L4 header checksum
>>>>>> calling skb_transport_header() with an uninitialized transport header.
>>>>>> This can potentially lead to a OOB write.
>>>>>
>>>>> Not really.  later fragments have l4_proto set to NEXTHDR_FRAGMENT
>>>>> and so no writes will be performed.  See parse_ipv6hdr().
>>>>>
>>>>
>>>> Oh, I didn't notice that. In that case, this is harmless. I will update
>>>> the commit message, thanks!
>>>>
>>>
>>> [...]
>>>
>>>>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>>>>> index dc5ff859f114..a14668565ec4 100644
>>>>>> --- a/net/openvswitch/actions.c
>>>>>> +++ b/net/openvswitch/actions.c
>>>>>> @@ -395,7 +395,7 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
>>>>>>     			  __be32 addr[4], const __be32 new_addr[4],
>>>>>>     			  bool recalculate_csum)
>>>>>>     {
>>>>>> -	if (recalculate_csum)
>>>>>> +	if (recalculate_csum && skb_transport_header_was_set(skb))
>>>>>>     		update_ipv6_checksum(skb, l4_proto, addr, new_addr);
>>>>> I'd suggest moving the check into update_ipv6_checksum() and maybe add
>>>>> a small comment that the check is only to avoid warning on reading the
>>>>> offset that wasn't set, offset must always be set for any l4_proto for
>>>>> which the checksum is actually getting updated.
>>>>>
>>>>
>>>> Will do that. Thanks Ilya.
>>> Actually, it might be better to check for NEXTHDR_FRAGMENT and return
>>> early only in this case.  This way if we ever have the offset missing
>>> for any l4_ptoto that needs it, we'll still get a warning instead of
>>> silently not updating the checksum.  This will be close to what the
>>> update_ip_l4_checksum() is doing.
>>>
>>> At the same time, update_ip_l4_checksum() is able to get the transport
>>> offset without a warning, so another alternative is to actually set the
>>> transport offset in parse_ipv6hdr() to something like frag_off, which
>>> would closer match the behavior of ipv4 where check_iphdr() sets the
>>> transport header unconditionally to the next byte after the network header.
>>> This is fine because transport headers are never accessed when they are
>>> not set in the key.  The network checksum update cases are the only ones
>>> where the actions are even allowed to execute and touch higher level
>>> headers that may not be present in the packet.  At the same time the
>>> frag_off is kind of an arbitrary value, so I'm not sure.
>>>
>>> WDYT?
>>
>> I noticed this but IMHO it is a worse pattern. In the sense that one
>> persone in the future might use the transport header without noticing
>> this and the warning won't show up because the transport header will be
>> set incorrectly. This could cause an actual OOB.
> 
> Yeah, that's true.  It's better to keep the transport header unset if
> there is actually no transport header.  We may also adjust the ipv4 path
> to avoid having it set for later fragments, but that's unrelated for the
> issue at hand.
> 

I do not mind giving a try to that in net-next. Good, so I will send the 
v2 with the proper check and commit message.

Thanks,
Fernando.

>> I will trust your taste in this matter since I am not that familiar with
>> the code.
> 
> Let's just go with the check inside update_ipv6_checksum() before reading
> the value.  Seems like the best option.
> 
> Best regards, Ilya Maximets.
> 
>>
>> Thanks,
>> Fernando.
>>
>>> Eelco, Aaron, do you maybe have a preference?
>>>
>>> Best regards, Ilya Maximets.
>>
>