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

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

[Hyunwoo Kim]

The locally generated output path reads the egress network device from
the route attached to the skb (skb_dst(skb)->dev, skb_dst_dev() or
rt->dst.dev) and uses it as the 'out' device argument of an
NF_HOOK()/nf_hook() call, or for direct field reads, without holding
rcu_read_lock().

dst->dev is protected by RCU. When a device is unregistered its value is
replaced with blackhole_netdev and the previous device is freed after an
RCU grace period. A section that reads dst->dev and uses that pointer
must therefore hold rcu_read_lock(). Otherwise a LOCAL_OUT / POST_ROUTING
hook consumer (nft meta oif, selinux_ip_postroute_compat, etc.) or an
early field read can reference a device that is no longer valid when the
egress device is unregistered concurrently with transmission.

Rather than taking the lock in each dst->output leaf, take it once at the
common ip_local_out() and ip6_local_out() level. This covers
__ip_local_out() / __ip6_local_out() (the LOCAL_OUT hook) and
dst_output(), and therefore ip_output(), ip_mc_output(), ip_mr_output(),
xfrm4_output() and vrf_output(), as well as the IPv6 leaves ip6_output(),
ip6_mr_output(), xfrm6_output() and vrf_output6(), in one place.

raw_send_hdrinc() and rawv6_send_hdrinc() do not go through
ip_local_out() / ip6_local_out(); they run their own NF_HOOK() and also
read the device (mtu, LL_RESERVED_SPACE(), needed_tailroom) before that
hook, so they take their own rcu_read_lock(). The device fields are read
under a short rcu_read_lock() at function entry that is dropped before
the blocking sock_alloc_send_skb(); the NF_HOOK() itself runs under
rcu_read_lock() (added in raw_send_hdrinc(), already present in
rawv6_send_hdrinc()).

xfrm_output_resume() is left unchanged. It reads the device locklessly
and is only reached either under the rcu_read_lock() held by its
dst_output() caller (now including ip_local_out() / ip6_local_out() for
locally generated traffic) or with softirqs disabled: the async crypto
completion runs in BH-off context (cryptd and padata invoke it under
local_bh_disable(); hardware crypto drivers complete it from softirq),
and the IPTFS output runs from a HRTIMER_MODE_REL_SOFT timer. For the
same reason __ip_local_out() and __ip6_local_out() keep the lockless
skb_dst_dev() accessor: they are also reached from xfrm_output_resume()
via ->local_out() in BH-off context, where the rcu_dereference() based
accessor would trip CONFIG_PROVE_RCU.

Fixes: 4a6ce2b6f2ec ("net: introduce a new function dst_dev_put()")
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
Changes in v3:
- Take the lock once at ip_local_out() / ip6_local_out() instead of in
  each output leaf, and cover the IPv6 path as well (David Ahern, Ido
  Schimmel).
- Drop the xfrm_output_resume() change; its resumption paths already run
  with BH off (Herbert Xu). Its synchronous entry is reached under the
  rcu held by its dst_output() caller.
- Also protect the early device reads (mtu, headroom) in
  raw_send_hdrinc() / rawv6_send_hdrinc(), split so the lock is not held
  across the blocking allocation.
- The now redundant rcu_read_lock() nested in ip_output() / ip6_output()
  will be removed in a follow-up.
- v2: https://lore.kernel.org/all/ahveCu-zzFlpeVut@v4bel/

Changes in v2:
- Changed to a net-wide patch that also fixes the issue in
  raw_send_hdrinc(), xfrm_output_resume() and vrf_output().
- v1: https://lore.kernel.org/all/ahqIg6vURwYI0LJ5@v4bel/
---
 net/ipv4/ip_output.c   |  2 ++
 net/ipv4/raw.c         | 19 ++++++++++++++-----
 net/ipv6/output_core.c |  2 ++
 net/ipv6/raw.c         | 16 ++++++++++++----
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 5bcd73cbdb41c..26b51ef0763fa 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 68e88cb3e55cb..1b7b291410cf8 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -336,11 +336,20 @@ static int raw_send_hdrinc(struct sock *sk, struct flowi4 *fl4,
 	unsigned int iphlen;
 	int err;
 	struct rtable *rt = *rtp;
+	struct net_device *dev;
 	int hlen, tlen;
+	unsigned int mtu;
 
-	if (length > rt->dst.dev->mtu) {
+	rcu_read_lock();
+	dev = dst_dev_rcu(&rt->dst);
+	mtu = dev->mtu;
+	hlen = LL_RESERVED_SPACE(dev);
+	tlen = dev->needed_tailroom;
+	rcu_read_unlock();
+
+	if (length > mtu) {
 		ip_local_error(sk, EMSGSIZE, fl4->daddr, inet->inet_dport,
-			       rt->dst.dev->mtu);
+			       mtu);
 		return -EMSGSIZE;
 	}
 	if (length < sizeof(struct iphdr))
@@ -349,8 +358,6 @@ static int raw_send_hdrinc(struct sock *sk, struct flowi4 *fl4,
 	if (flags&MSG_PROBE)
 		goto out;
 
-	hlen = LL_RESERVED_SPACE(rt->dst.dev);
-	tlen = rt->dst.dev->needed_tailroom;
 	skb = sock_alloc_send_skb(sk,
 				  length + hlen + tlen + 15,
 				  flags & MSG_DONTWAIT, &err);
@@ -410,9 +417,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/ipv6/output_core.c b/net/ipv6/output_core.c
index 64b1eeb79b572..ac058af198986 100644
--- a/net/ipv6/output_core.c
+++ b/net/ipv6/output_core.c
@@ -124,9 +124,11 @@ int ip6_local_out(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
 	int err;
 
+	rcu_read_lock();
 	err = __ip6_local_out(net, sk, skb);
 	if (likely(err == 1))
 		err = dst_output(net, sk, skb);
+	rcu_read_unlock();
 
 	return err;
 }
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 3cc58698cbbd3..73ac6ef2d3168 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -601,11 +601,19 @@ static int rawv6_send_hdrinc(struct sock *sk, struct msghdr *msg, int length,
 	struct sk_buff *skb;
 	int err;
 	struct rt6_info *rt = dst_rt6_info(*dstp);
-	int hlen = LL_RESERVED_SPACE(rt->dst.dev);
-	int tlen = rt->dst.dev->needed_tailroom;
+	struct net_device *dev;
+	int hlen, tlen;
+	unsigned int mtu;
 
-	if (length > rt->dst.dev->mtu) {
-		ipv6_local_error(sk, EMSGSIZE, fl6, rt->dst.dev->mtu);
+	rcu_read_lock();
+	dev = dst_dev_rcu(&rt->dst);
+	mtu = dev->mtu;
+	hlen = LL_RESERVED_SPACE(dev);
+	tlen = dev->needed_tailroom;
+	rcu_read_unlock();
+
+	if (length > mtu) {
+		ipv6_local_error(sk, EMSGSIZE, fl6, mtu);
 		return -EMSGSIZE;
 	}
 	if (length < sizeof(struct ipv6hdr))
-- 
2.43.0

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

[Paolo Abeni]

On 6/12/26 1:46 AM, Hyunwoo Kim wrote:
> The locally generated output path reads the egress network device from
> the route attached to the skb (skb_dst(skb)->dev, skb_dst_dev() or
> rt->dst.dev) and uses it as the 'out' device argument of an
> NF_HOOK()/nf_hook() call, or for direct field reads, without holding
> rcu_read_lock().
> 
> dst->dev is protected by RCU. When a device is unregistered its value is
> replaced with blackhole_netdev and the previous device is freed after an
> RCU grace period. A section that reads dst->dev and uses that pointer
> must therefore hold rcu_read_lock(). Otherwise a LOCAL_OUT / POST_ROUTING
> hook consumer (nft meta oif, selinux_ip_postroute_compat, etc.) or an
> early field read can reference a device that is no longer valid when the
> egress device is unregistered concurrently with transmission.
> 
> Rather than taking the lock in each dst->output leaf, take it once at the
> common ip_local_out() and ip6_local_out() level. This covers
> __ip_local_out() / __ip6_local_out() (the LOCAL_OUT hook) and
> dst_output(), and therefore ip_output(), ip_mc_output(), ip_mr_output(),
> xfrm4_output() and vrf_output(), as well as the IPv6 leaves ip6_output(),
> ip6_mr_output(), xfrm6_output() and vrf_output6(), in one place.
> 
> raw_send_hdrinc() and rawv6_send_hdrinc() do not go through
> ip_local_out() / ip6_local_out(); they run their own NF_HOOK() and also
> read the device (mtu, LL_RESERVED_SPACE(), needed_tailroom) before that
> hook, so they take their own rcu_read_lock(). The device fields are read
> under a short rcu_read_lock() at function entry that is dropped before
> the blocking sock_alloc_send_skb(); the NF_HOOK() itself runs under
> rcu_read_lock() (added in raw_send_hdrinc(), already present in
> rawv6_send_hdrinc()).
> 
> xfrm_output_resume() is left unchanged. It reads the device locklessly
> and is only reached either under the rcu_read_lock() held by its
> dst_output() caller (now including ip_local_out() / ip6_local_out() for
> locally generated traffic) or with softirqs disabled: the async crypto
> completion runs in BH-off context (cryptd and padata invoke it under
> local_bh_disable(); hardware crypto drivers complete it from softirq),
> and the IPTFS output runs from a HRTIMER_MODE_REL_SOFT timer. For the
> same reason __ip_local_out() and __ip6_local_out() keep the lockless
> skb_dst_dev() accessor: they are also reached from xfrm_output_resume()
> via ->local_out() in BH-off context, where the rcu_dereference() based
> accessor would trip CONFIG_PROVE_RCU.
> 
> Fixes: 4a6ce2b6f2ec ("net: introduce a new function dst_dev_put()")
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> ---
> Changes in v3:
> - Take the lock once at ip_local_out() / ip6_local_out() instead of in
>   each output leaf, and cover the IPv6 path as well (David Ahern, Ido
>   Schimmel).
> - Drop the xfrm_output_resume() change; its resumption paths already run
>   with BH off (Herbert Xu). Its synchronous entry is reached under the
>   rcu held by its dst_output() caller.
> - Also protect the early device reads (mtu, headroom) in
>   raw_send_hdrinc() / rawv6_send_hdrinc(), split so the lock is not held
>   across the blocking allocation.
> - The now redundant rcu_read_lock() nested in ip_output() / ip6_output()
>   will be removed in a follow-up.
> - v2: https://lore.kernel.org/all/ahveCu-zzFlpeVut@v4bel/
> 
> Changes in v2:
> - Changed to a net-wide patch that also fixes the issue in
>   raw_send_hdrinc(), xfrm_output_resume() and vrf_output().
> - v1: https://lore.kernel.org/all/ahqIg6vURwYI0LJ5@v4bel/
> ---
>  net/ipv4/ip_output.c   |  2 ++
>  net/ipv4/raw.c         | 19 ++++++++++++++-----
>  net/ipv6/output_core.c |  2 ++
>  net/ipv6/raw.c         | 16 ++++++++++++----
>  4 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 5bcd73cbdb41c..26b51ef0763fa 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();

IN RH we are observing small but measurable and reproducible regressions
due to increased number of rcu lock pair in the fast-path.
ip_local_out() is invoked by __ip_queue_xmit() already under the RCU lock.

I think it could be usedful either providing an _rcu less variant and
use the latter in __ip_queue_xmit(), or open-code ip_local_out() there.

AFAICS the ipv6 counter part is not in the local output fastpath.

/P