Re: [PATCH net] net: loopback: Avoid sending IP packets without an Ethernet header

[Ido Schimmel <idosch@nvidia.com>]

On Thu, Feb 20, 2025 at 11:40:07AM +0100, Eric Dumazet wrote:
> On Thu, Feb 20, 2025 at 8:26 AM Ido Schimmel <idosch@nvidia.com> wrote:
> > diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> > index c8840c3b9a1b..f1d68153987e 100644
> > --- a/drivers/net/loopback.c
> > +++ b/drivers/net/loopback.c
> > @@ -244,8 +244,22 @@ static netdev_tx_t blackhole_netdev_xmit(struct sk_buff *skb,
> >         return NETDEV_TX_OK;
> >  }
> >
> > +static int blackhole_neigh_output(struct neighbour *n, struct sk_buff *skb)
> > +{
> > +       kfree_skb(skb);
> 
> If there is any risk of this being hit often, I would probably use the
> recent SKB_DROP_REASON_BLACKHOLE

Not very often. About 10 times while running the reproducer I shared
here:
https://lore.kernel.org/netdev/Z7D9cR22BDPN7WSJ@shredder/

In line with the original report:
https://github.com/siderolabs/talos/issues/9837#issuecomment-2642116378

> (feel free to resubmit
> https://lore.kernel.org/netdev/20250212164323.2183023-1-edumazet@google.com/T/#mbb8d4b0779cb8f0654a382772c943af5389606ea
> ?)

Can we do it in net-next?

A few questions / suggestions regarding the patch:

1. Can we use it for IPv4 as well? I tested the following:

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 753704f75b2c..2aeab70c1cb5 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -966,6 +966,7 @@ static int ip_error(struct sk_buff *skb)
 
 	switch (rt->dst.error) {
 	case EINVAL:
+		SKB_DR_SET(reason, BLACKHOLE);
 	default:
 		goto out;
 	case EHOSTUNREACH:

2. Given that this reason is going to be triggered both for
user-installed blackhole routes and dst entries being destroyed how
about adjusting the comment? Otherwise I think it will be confusing for
users who didn't install a blackhole route. Something like:

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index f1d68153987e..cb269b3251d4 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -239,14 +239,13 @@ struct pernet_operations __net_initdata loopback_net_ops = {
 static netdev_tx_t blackhole_netdev_xmit(struct sk_buff *skb,
 					 struct net_device *dev)
 {
-	kfree_skb(skb);
-	net_warn_ratelimited("%s(): Dropping skb.\n", __func__);
+	kfree_skb_reason(skb, SKB_DROP_REASON_BLACKHOLE);
 	return NETDEV_TX_OK;
 }
 
 static int blackhole_neigh_output(struct neighbour *n, struct sk_buff *skb)
 {
-	kfree_skb(skb);
+	kfree_skb_reason(skb, SKB_DROP_REASON_BLACKHOLE);
 	return 0;
 }
 
diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index de42577f16dd..0ef6869dbd1b 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -556,7 +556,8 @@ enum skb_drop_reason {
 	 */
 	SKB_DROP_REASON_BRIDGE_INGRESS_STP_STATE,
 	/**
-	 * @SKB_DROP_REASON_BLACKHOLE: blackhole route.
+	 * @SKB_DROP_REASON_BLACKHOLE: blackhole route or dst entry being
+	 * destroyed.
 	 */
 	SKB_DROP_REASON_BLACKHOLE,
 	/**

> Otherwise, this looks good to me, thanks !
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks!