Jesper Dangaard Brouer wrote:

On Fri, 30 Oct 2020 13:36:05 -0700
John Fastabend [off-list ref] wrote:

quoted

Jesper Dangaard Brouer wrote:

quoted

The use-case for dropping the MTU check when TC-BPF does redirect to
ingress, is described by Eyal Birger in email[0]. The summary is the
ability to increase packet size (e.g. with IPv6 headers for NAT64) and
ingress redirect packet and let normal netstack fragment packet as needed.

[0] https://lore.kernel.org/netdev/CAHsH6Gug-hsLGHQ6N0wtixdOa85LDZ3HNRHVd0opR=19Qo4W4Q@mail.gmail.com/ (local)

V4:
 - Keep net_device "up" (IFF_UP) check.
 - Adjustment to handle bpf_redirect_peer() helper

Signed-off-by: Jesper Dangaard Brouer <redacted>
---
 include/linux/netdevice.h |   31 +++++++++++++++++++++++++++++--
 net/core/dev.c            |   19 ++-----------------
 net/core/filter.c         |   14 +++++++++++---
 3 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 964b494b0e8d..bd02ddab8dfe 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -3891,11 +3891,38 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
 bool is_skb_forwardable(const struct net_device *dev,
 			const struct sk_buff *skb);
 
+static __always_inline bool __is_skb_forwardable(const struct net_device *dev,
+						 const struct sk_buff *skb,
+						 const bool check_mtu)  

It looks like if check_mtu=false then this is just an interface up check.
Can we leave is_skb_forwardable logic alone and just change the spots where
this is called with false to something with a name that describes the check,
such as is_dev_up(dev). I think it will make this change smaller and the
code easier to read. Did I miss something?

People should realized that this is constructed such, the compiler will
compile-time remove the actual argument (the const bool check_mtu).
And this propagates also to ____dev_forward_skb() where the call places
are also inlined.

The comment was about human reading the code not what gets generated
by the compiler.

Yes, this (check_mtu=false) is basically an interface up check, but the
only place it is used directly is in the ndo_get_peer_dev() case, and
reading the code I find it more readable that is says
__is_skb_forwardable because this is used as part of a forwarding step,
and is_dev_up() doesn't convey the intent in this use-case.

OK.

[...]

quoted

diff --git a/net/core/filter.c b/net/core/filter.c
index bd4a416bd9ad..71b78b8d443c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c

@@ -2083,13 +2083,21 @@ static const struct bpf_func_proto bpf_csum_level_proto = {
 
 static inline int __bpf_rx_skb(struct net_device *dev, struct sk_buff *skb)
 {
-	return dev_forward_skb(dev, skb);
+	int ret = ____dev_forward_skb(dev, skb, false);
+
+	if (likely(!ret)) {
+		skb->protocol = eth_type_trans(skb, dev);
+		skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
+		ret = netif_rx(skb);
+	}
+
+	return ret;
 }

I'm replicating two lines from dev_forward_skb(), but I couldn't find a
way to avoid this, without causing larger code changes (and slower code).

OK looks good to me then.

quoted

Other than style aspects it looks correct to me.

quoted

 	if (skb_orphan_frags(skb, GFP_ATOMIC) ||
-	    unlikely(!is_skb_forwardable(dev, skb))) {
+	    unlikely(!__is_skb_forwardable(dev, skb, check_mtu))) {
 		atomic_long_inc(&dev->rx_dropped);
 		kfree_skb(skb);
 		return NET_RX_DROP;

diff --git a/net/core/dev.c b/net/core/dev.c
index 9499a414d67e..445ccf92c149 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -2188,28 +2188,13 @@ static inline void net_timestamp_set(struct sk_buff *skb)
   

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer