Re: [PATCH net-next 5/9] sit: convert 6RD configuration to RCU protection

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] check_6rd() now performs `ip6rd =
  rcu_dereference(tunnel->ip6rd)` (net/ipv6/sit.c:849), but check_6rd()…
- [Low] The patch changes SIOCDEL6RD behaviour beyond the advertised RCU
  conversion: ipip6_tunnel_clone_6rd() now, when an old 6RD…

--

sit: convert 6RD configuration to RCU protection

This converts tunnel->ip6rd from an embedded struct into an RCU protected
pointer, so updates via ipip6_tunnel_update_6rd() and
ipip6_tunnel_clone_6rd() allocate a new struct ip_tunnel_6rd_parm, publish
it with rcu_assign_pointer() and free the old one with kfree_rcu().
Readers in check_6rd(), only_dnatted(), ipip6_tunnel_get6rd() and
ipip6_fill_info() are updated accordingly.

[Severity: Low]
Is the new dst_cache_reset() plus netdev_state_change() in
ipip6_tunnel_clone_6rd() intended as part of this conversion?

Before this patch, the SIOCDEL6RD path did neither:

  ipip6_tunnel_6rdctl()
    ipip6_tunnel_clone_6rd()   /* just overwrote t->ip6rd fields */

Only ipip6_tunnel_update_6rd() invalidated the destination cache and
notified userspace.  Since ipip6_tunnel_xmit() derives the IPv4
destination from the 6RD configuration via try_6rd() and then consults
dst_cache_get_ip4(&tunnel->dst_cache, ...), the added reset changes where
packets go after a 6RD deletion, and netdev_state_change() ->
netif_state_change() emits an extra NETDEV_CHANGE plus RTM_NEWLINK that
link watchers will see.

Would it make sense to split that out into its own patch, with a Fixes:
tag if it is fixing stale routing after SIOCDEL6RD?  The changelog
currently only describes the allocation and publish conversion.

[ ... ]

[Severity: Medium]
Is plain rcu_dereference() the right accessor here?  The changelog says:

  "Readers in check_6rd() and only_dnatted() use rcu_dereference() under
   existing RCU read lock"

That holds for the receive path, but check_6rd() is also reached from
transmit:

  netdev_start_xmit()
    sit_tunnel_xmit()
      ipip6_tunnel_xmit()
        try_6rd()
          check_6rd()
            rcu_dereference(tunnel->ip6rd)

and the xmit callers do not hold rcu_read_lock().  __dev_queue_xmit()
takes rcu_read_lock_bh(), which acquires rcu_bh_lock_map, and the
AF_PACKET qdisc bypass path __dev_direct_xmit() only does
local_bh_disable():

net/core/dev.c:__dev_direct_xmit() {
	...
	local_bh_disable();

	dev_xmit_recursion_inc();
	HARD_TX_LOCK(dev, txq, smp_processor_id());
	...
}

rcu_dereference() expands to rcu_dereference_check(p, 0), whose condition
is rcu_read_lock_held():

kernel/rcu/update.c:rcu_read_lock_held() {
	...
	return lock_is_held(&rcu_lock_map);
}

so on a CONFIG_PROVE_RCU kernel, would sending a packet on a sit device
from these contexts produce a "suspicious rcu_dereference_check() usage"
splat?  Object lifetime looks fine, since BH-disabled regions are RCU
read-side sections, but the annotation seems off.

Would rcu_dereference_bh() (or rcu_dereference_check() with
rcu_read_lock_bh_held()) be more accurate, or alternatively an explicit
rcu_read_lock() in ipip6_tunnel_xmit(), like vxlan_xmit() does before its
RCU lookups?  The same question applies to the new rcu_dereference() in
only_dnatted().

[ ... ]

Related to the comment above: SIOCDEL6RD can now also fail with -ENOMEM,
where it previously always returned 0.  Is that intentional?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907075846.2913645-1-edumazet%40google.com