Re: [PATCH net v2] net: protect egress device access in the output path with rcu_read_lock

[David Ahern <dsahern@kernel.org>]

On 6/3/26 6:58 AM, Hyunwoo Kim wrote:
> On Tue, Jun 02, 2026 at 10:24:09AM -0600, David Ahern wrote:
>> On 6/2/26 9:45 AM, Ido Schimmel wrote:
>>> On Sun, May 31, 2026 at 04:06:50PM +0900, Hyunwoo Kim wrote:
>>>>  drivers/net/vrf.c       | 16 +++++++++++-----
>>>>  net/ipv4/ip_output.c    | 27 +++++++++++++++++++--------
>>>>  net/ipv4/raw.c          |  4 +++-
>>>>  net/ipv4/xfrm4_output.c | 13 +++++++++----
>>>>  net/xfrm/xfrm_output.c  |  4 +++-
>>>>  5 files changed, 45 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
>>>> index 46209917ae4d..e9a1dd961805 100644
>>>> --- a/drivers/net/vrf.c
>>>> +++ b/drivers/net/vrf.c
>>>> @@ -833,17 +833,23 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s
>>>>  
>>>>  static int vrf_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>>>>  {
>>>> -	struct net_device *dev = skb_dst(skb)->dev;
>>>> +	struct net_device *dev;
>>>> +	int ret;
>>>> +
>>>> +	rcu_read_lock();
>>>> +	dev = skb_dst_dev_rcu(skb);
>>>>  
>>>>  	IP_UPD_PO_STATS(net, IPSTATS_MIB_OUT, skb->len);
>>>>  
>>>>  	skb->dev = dev;
>>>>  	skb->protocol = htons(ETH_P_IP);
>>>>  
>>>> -	return NF_HOOK_COND(NFPROTO_IPV4, NF_INET_POST_ROUTING,
>>>> -			    net, sk, skb, NULL, dev,
>>>> -			    vrf_finish_output,
>>>> -			    !(IPCB(skb)->flags & IPSKB_REROUTED));
>>>> +	ret = NF_HOOK_COND(NFPROTO_IPV4, NF_INET_POST_ROUTING,
>>>> +			   net, sk, skb, NULL, dev,
>>>> +			   vrf_finish_output,
>>>> +			   !(IPCB(skb)->flags & IPSKB_REROUTED));
>>>> +	rcu_read_unlock();
>>>> +	return ret;
>>>>  }
>>>
>>> Patch LGTM, thanks, but what about the IPv6 counterpart (vrf_output6())
> 
> I left ipv6 out because its callers (ip6_send_skb, ip6_xmit, etc.)
> already hold rcu. Though some niche paths may not. (maybe rxe_send?)
> 
>>> and the other similar existing issues that Sashiko is flagging?
> 
> I couldn't find Sashiko's review. Could you share a url?
> 
>>
>> Common locking at a higher level than dst->output seems like a better
>> approach. This function is called under rcu_read_lock for some paths.
> 
> Adding rcu to ip_local_out would protect all functions reached via
> dst_output in one place; raw_send_hdrinc and xfrm_output_resume still
> need their own patches. What do you think of this diff?
> 
> As a follow-up, the rcu in ip_output etc. then becomes redundant and
> should be removed.
> 
> 
> Best regards,
> Hyunwoo Kim
> 
> ---
> 
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 5bcd73cbdb41..26b51ef0763f 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -126,9 +126,11 @@ int ip_local_out(struct net *net, struct sock *sk, struct sk_buff *skb)
>  {
>  	int err;
> 
> +	rcu_read_lock();
>  	err = __ip_local_out(net, sk, skb);
>  	if (likely(err == 1))
>  		err = dst_output(net, sk, skb);
> +	rcu_read_unlock();
> 
>  	return err;
>  }
> diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
> index 68e88cb3e55c..555e62eacdc6 100644
> --- a/net/ipv4/raw.c
> +++ b/net/ipv4/raw.c
> @@ -410,9 +410,11 @@ static int raw_send_hdrinc(struct sock *sk, struct flowi4 *fl4,
>  				skb_transport_header(skb))->type);
>  	}
> 
> +	rcu_read_lock();
>  	err = NF_HOOK(NFPROTO_IPV4, NF_INET_LOCAL_OUT,
> -		      net, sk, skb, NULL, rt->dst.dev,
> +		      net, sk, skb, NULL, skb_dst_dev_rcu(skb),
>  		      dst_output);
> +	rcu_read_unlock();
>  	if (err > 0)
>  		err = net_xmit_errno(err);
>  	if (err)
> diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> index cc35c2fcbbe0..c5ac360da38d 100644
> --- a/net/xfrm/xfrm_output.c
> +++ b/net/xfrm/xfrm_output.c
> @@ -594,6 +594,7 @@ int xfrm_output_resume(struct sock *sk, struct sk_buff *skb, int err)
>  {
>  	struct net *net = xs_net(skb_dst(skb)->xfrm);
> 
> +	rcu_read_lock();
>  	while (likely((err = xfrm_output_one(skb, err)) == 0)) {
>  		nf_reset_ct(skb);
> 
> @@ -601,12 +602,14 @@ int xfrm_output_resume(struct sock *sk, struct sk_buff *skb, int err)
>  		if (unlikely(err != 1))
>  			goto out;
> 
> -		if (!skb_dst(skb)->xfrm)
> -			return dst_output(net, sk, skb);
> +		if (!skb_dst(skb)->xfrm) {
> +			err = dst_output(net, sk, skb);
> +			goto out;
> +		}
> 
>  		err = nf_hook(skb_dst(skb)->ops->family,
>  			      NF_INET_POST_ROUTING, net, sk, skb,
> -			      NULL, skb_dst(skb)->dev, xfrm_output2);
> +			      NULL, skb_dst_dev_rcu(skb), xfrm_output2);
>  		if (unlikely(err != 1))
>  			goto out;
>  	}
> @@ -615,6 +618,7 @@ int xfrm_output_resume(struct sock *sk, struct sk_buff *skb, int err)
>  		err = 0;
> 
>  out:
> +	rcu_read_unlock();
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(xfrm_output_resume);

I prefer this over locking in each of the output functions. IPv6 should
be updated as well.