[PATCH net] neigh: let neigh_xmit take skb ownership

[PATCH net] neigh: let neigh_xmit take skb ownership

[Florian Westphal]

neigh_xmit always releases the skb, except when no neighbour table is
found. But even the first added user of neigh_xmit (mpls) relied on
neigh_xmit to release the skb (or queue it for tx).

sashiko reported:
 If neigh_xmit() is called with an uninitialized neighbor table (for
 example, NEIGH_ND_TABLE when IPv6 is disabled), it returns -EAFNOSUPPORT
 and bypasses its internal out_kfree_skb error path.  Because the return
 value of neigh_xmit() is ignored here, does this leak the SKB?

Assume full ownership and remove the last code path that doesn't
xmit or free skb.

Fixes: 4fd3d7d9e868 ("neigh: Add helper function neigh_xmit")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 could followup in -next to make it "void", existing
 callers either ignore retval or do:
 if (err)
 	net_dbg_ratelimited("%s: ...

 net/core/neighbour.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index c56a4e7bf790..5a9cc7268521 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -3211,8 +3211,10 @@ int neigh_xmit(int index, struct net_device *dev,
 
 		rcu_read_lock();
 		tbl = rcu_dereference(neigh_tables[index]);
-		if (!tbl)
-			goto out_unlock;
+		if (!tbl) {
+			rcu_read_unlock();
+			goto out_kfree_skb;
+		}
 		if (index == NEIGH_ARP_TABLE) {
 			u32 key = *((u32 *)addr);
 
@@ -3228,7 +3230,6 @@ int neigh_xmit(int index, struct net_device *dev,
 			goto out_kfree_skb;
 		}
 		err = READ_ONCE(neigh->output)(neigh, skb);
-out_unlock:
 		rcu_read_unlock();
 	}
 	else if (index == NEIGH_LINK_TABLE) {
@@ -3238,11 +3239,10 @@ int neigh_xmit(int index, struct net_device *dev,
 			goto out_kfree_skb;
 		err = dev_queue_xmit(skb);
 	}
-out:
 	return err;
 out_kfree_skb:
 	kfree_skb(skb);
-	goto out;
+	return err;
 }
 EXPORT_SYMBOL(neigh_xmit);
 
-- 
2.53.0

Re: [PATCH net] neigh: let neigh_xmit take skb ownership

[Kuniyuki Iwashima]

On Fri, Apr 24, 2026 at 8:00 AM Florian Westphal <fw@strlen.de> wrote:
>
> neigh_xmit always releases the skb, except when no neighbour table is
> found. But even the first added user of neigh_xmit (mpls) relied on
> neigh_xmit to release the skb (or queue it for tx).
>
> sashiko reported:
>  If neigh_xmit() is called with an uninitialized neighbor table (for
>  example, NEIGH_ND_TABLE when IPv6 is disabled), it returns -EAFNOSUPPORT
>  and bypasses its internal out_kfree_skb error path.  Because the return
>  value of neigh_xmit() is ignored here, does this leak the SKB?
>
> Assume full ownership and remove the last code path that doesn't
> xmit or free skb.
>
> Fixes: 4fd3d7d9e868 ("neigh: Add helper function neigh_xmit")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  could followup in -next to make it "void",

Makes sense.

Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>

Re: [PATCH net] neigh: let neigh_xmit take skb ownership

[Ido Schimmel]

On Fri, Apr 24, 2026 at 04:58:38PM +0200, Florian Westphal wrote:
> neigh_xmit always releases the skb, except when no neighbour table is
> found. But even the first added user of neigh_xmit (mpls) relied on
> neigh_xmit to release the skb (or queue it for tx).
> 
> sashiko reported:
>  If neigh_xmit() is called with an uninitialized neighbor table (for
>  example, NEIGH_ND_TABLE when IPv6 is disabled), it returns -EAFNOSUPPORT
>  and bypasses its internal out_kfree_skb error path.  Because the return
>  value of neigh_xmit() is ignored here, does this leak the SKB?
> 
> Assume full ownership and remove the last code path that doesn't
> xmit or free skb.
> 
> Fixes: 4fd3d7d9e868 ("neigh: Add helper function neigh_xmit")
> Signed-off-by: Florian Westphal <fw@strlen.de>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>