LinuxLists.cc - [PATCH v2 net-next] packet: add a generic drop reason for receive

2023-12-01 17:43:32

Subject: [PATCH v2 net-next] packet: add a generic drop reason for receive

Commit da37845fdce2 ("packet: uses kfree_skb() for errors.") switches
from consume_skb to kfree_skb to improve error handling. However, this
could bring a lot of noises when we monitor real packet drops in
kfree_skb[1], because in tpacket_rcv or packet_rcv only packet clones
can be freed, not actual packets.

Adding a generic drop reason to allow distinguish these "clone drops".

[1]: https://lore.kernel.org/netdev/CABWYdi00L+O30Q=Zah28QwZ_5RU-xcxLFUK2Zj08A8MrLk9jzg@mail.gmail.com/
Fixes: da37845fdce2 ("packet: uses kfree_skb() for errors.")
Suggested-by: Eric Dumazet <[email protected]>
Signed-off-by: Yan Zhai <[email protected]>
---
include/net/dropreason-core.h | 6 ++++++
net/packet/af_packet.c | 22 +++++++++++++---------
2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 3c70ad53a49c..278e4c7d465c 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -86,6 +86,7 @@
FN(IPV6_NDISC_NS_OTHERHOST) \
FN(QUEUE_PURGE) \
FN(TC_ERROR) \
+ FN(PACKET_SOCK_ERROR) \
FNe(MAX)

/**
@@ -378,6 +379,11 @@ enum skb_drop_reason {
SKB_DROP_REASON_QUEUE_PURGE,
/** @SKB_DROP_REASON_TC_ERROR: generic internal tc error. */
SKB_DROP_REASON_TC_ERROR,
+ /**
+ * @SKB_DROP_REASON_PACKET_SOCK_ERROR: generic packet socket errors
+ * after its filter matches an incoming packet.
+ */
+ SKB_DROP_REASON_PACKET_SOCK_ERROR,
/**
* @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
* shouldn't be used as a real 'reason' - only for tracing code gen
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index a84e00b5904b..0a7c05d8fe9f 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2128,6 +2128,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
int skb_len = skb->len;
unsigned int snaplen, res;
bool is_drop_n_account = false;
+ enum skb_drop_reason drop_reason = SKB_CONSUMED;

if (skb->pkt_type == PACKET_LOOPBACK)
goto drop;
@@ -2161,6 +2162,10 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
res = run_filter(skb, sk, snaplen);
if (!res)
goto drop_n_restore;
+
+ /* skb will only be "consumed" not "dropped" before this */
+ drop_reason = SKB_DROP_REASON_PACKET_SOCK_ERROR;
+
if (snaplen > res)
snaplen = res;

@@ -2227,10 +2232,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
skb->len = skb_len;
}
drop:
- if (!is_drop_n_account)
- consume_skb(skb);
- else
- kfree_skb(skb);
+ kfree_skb_reason(skb, drop_reason);
return 0;
}

@@ -2253,6 +2255,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
bool is_drop_n_account = false;
unsigned int slot_id = 0;
int vnet_hdr_sz = 0;
+ enum skb_drop_reason drop_reason = SKB_CONSUMED;

/* struct tpacket{2,3}_hdr is aligned to a multiple of TPACKET_ALIGNMENT.
* We may add members to them until current aligned size without forcing
@@ -2355,6 +2358,10 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
vnet_hdr_sz = 0;
}
}
+
+ /* skb will only be "consumed" not "dropped" before this */
+ drop_reason = SKB_DROP_REASON_PACKET_SOCK_ERROR;
+
spin_lock(&sk->sk_receive_queue.lock);
h.raw = packet_current_rx_frame(po, skb,
TP_STATUS_KERNEL, (macoff+snaplen));
@@ -2498,10 +2505,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
skb->len = skb_len;
}
drop:
- if (!is_drop_n_account)
- consume_skb(skb);
- else
- kfree_skb(skb);
+ kfree_skb_reason(skb, drop_reason);
return 0;

drop_n_account:
@@ -2510,7 +2514,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
is_drop_n_account = true;

sk->sk_data_ready(sk);
- kfree_skb(copy_skb);
+ kfree_skb_reason(copy_skb, drop_reason);
goto drop_n_restore;
}

--
2.30.2

2023-12-01 17:51:28

by Eric Dumazet

[permalink] [raw]

Subject: Re: [PATCH v2 net-next] packet: add a generic drop reason for receive

On Fri, Dec 1, 2023 at 6:43 PM Yan Zhai <[email protected]> wrote:
>
> Commit da37845fdce2 ("packet: uses kfree_skb() for errors.") switches
> from consume_skb to kfree_skb to improve error handling. However, this
> could bring a lot of noises when we monitor real packet drops in
> kfree_skb[1], because in tpacket_rcv or packet_rcv only packet clones
> can be freed, not actual packets.
>
> Adding a generic drop reason to allow distinguish these "clone drops".
>
> [1]: https://lore.kernel.org/netdev/CABWYdi00L+O30Q=Zah28QwZ_5RU-xcxLFUK2Zj08A8MrLk9jzg@mail.gmail.com/
> Fixes: da37845fdce2 ("packet: uses kfree_skb() for errors.")
> Suggested-by: Eric Dumazet <[email protected]>
> Signed-off-by: Yan Zhai <[email protected]>
> ---
> include/net/dropreason-core.h | 6 ++++++
> net/packet/af_packet.c | 22 +++++++++++++---------
> 2 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> index 3c70ad53a49c..278e4c7d465c 100644
> --- a/include/net/dropreason-core.h
> +++ b/include/net/dropreason-core.h
> @@ -86,6 +86,7 @@
> FN(IPV6_NDISC_NS_OTHERHOST) \
> FN(QUEUE_PURGE) \
> FN(TC_ERROR) \
> + FN(PACKET_SOCK_ERROR) \
> FNe(MAX)
>
> /**
> @@ -378,6 +379,11 @@ enum skb_drop_reason {
> SKB_DROP_REASON_QUEUE_PURGE,
> /** @SKB_DROP_REASON_TC_ERROR: generic internal tc error. */
> SKB_DROP_REASON_TC_ERROR,
> + /**
> + * @SKB_DROP_REASON_PACKET_SOCK_ERROR: generic packet socket errors
> + * after its filter matches an incoming packet.
> + */
> + SKB_DROP_REASON_PACKET_SOCK_ERROR,
> /**
> * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
> * shouldn't be used as a real 'reason' - only for tracing code gen
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index a84e00b5904b..0a7c05d8fe9f 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2128,6 +2128,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
> int skb_len = skb->len;
> unsigned int snaplen, res;
> bool is_drop_n_account = false;

Why keeping is_drop_n_account then ?

> + enum skb_drop_reason drop_reason = SKB_CONSUMED;
>
> if (skb->pkt_type == PACKET_LOOPBACK)
> goto drop;
> @@ -2161,6 +2162,10 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
> res = run_filter(skb, sk, snaplen);
> if (!res)
> goto drop_n_restore;
> +
> + /* skb will only be "consumed" not "dropped" before this */
> + drop_reason = SKB_DROP_REASON_PACKET_SOCK_ERROR;
> +
> if (snaplen > res)
> snaplen = res;
>
> @@ -2227,10 +2232,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
> skb->len = skb_len;
> }
> drop:
> - if (!is_drop_n_account)

Because after your patch it will be set but never read.

> - consume_skb(skb);
> - else
> - kfree_skb(skb);
> + kfree_skb_reason(skb, drop_reason);
> return 0;
> }
>
> @@ -2253,6 +2255,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> bool is_drop_n_account = false;

Same remark here.

> unsigned int slot_id = 0;
> int vnet_hdr_sz = 0;
> + enum skb_drop_reason drop_reason = SKB_CONSUMED;
>
> /* struct tpacket{2,3}_hdr is aligned to a multiple of TPACKET_ALIGNMENT.
> * We may add members to them until current aligned size without forcing
> @@ -2355,6 +2358,10 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> vnet_hdr_sz = 0;
> }
> }
> +
> + /* skb will only be "consumed" not "dropped" before this */
> + drop_reason = SKB_DROP_REASON_PACKET_SOCK_ERROR;
> +
> spin_lock(&sk->sk_receive_queue.lock);
> h.raw = packet_current_rx_frame(po, skb,
> TP_STATUS_KERNEL, (macoff+snaplen));
> @@ -2498,10 +2505,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> skb->len = skb_len;
> }
> drop:
> - if (!is_drop_n_account)
> - consume_skb(skb);
> - else
> - kfree_skb(skb);
> + kfree_skb_reason(skb, drop_reason);
> return 0;
>
> drop_n_account:
> @@ -2510,7 +2514,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> is_drop_n_account = true;
>
> sk->sk_data_ready(sk);
> - kfree_skb(copy_skb);
> + kfree_skb_reason(copy_skb, drop_reason);
> goto drop_n_restore;
> }
>
> --
> 2.30.2
>

2023-12-01 18:19:57

by Yan Zhai

[permalink] [raw]

Subject: Re: [PATCH v2 net-next] packet: add a generic drop reason for receive

On Fri, Dec 1, 2023 at 11:51 AM Eric Dumazet <[email protected]> wrote:
>
> > bool is_drop_n_account = false;
>
> Why keeping is_drop_n_account then ?

Good catch, thanks! Will send a v3 to fix up.
Meanwhile, I noticed it is compiled with the
-Wno-unused-but-set-variable flag, is there a reason why we disable
this warning?

Yan

2023-12-01 18:31:35

by Yan Zhai

[permalink] [raw]

Subject: Re: [PATCH v2 net-next] packet: add a generic drop reason for receive

On Fri, Dec 1, 2023 at 12:19 PM Yan Zhai <[email protected]> wrote:
>
> On Fri, Dec 1, 2023 at 11:51 AM Eric Dumazet <[email protected]> wrote:
> >
> > > bool is_drop_n_account = false;
> >
> > Why keeping is_drop_n_account then ?
>
> Good catch, thanks! Will send a v3 to fix up.
> Meanwhile, I noticed it is compiled with the
> -Wno-unused-but-set-variable flag, is there a reason why we disable
> this warning?

I found the answer in scripts/Makefile.extrawarn. Good to learn how this works!

>
> Yan

2023-12-06 02:26:48

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2 net-next] packet: add a generic drop reason for receive

Hi Yan,

kernel test robot noticed
[auto build test WARNING
url: If you fix the issue the same patch/commit), | Reported-by: kernel | Closes: on net-next/main]
hub.com/intel-lab-lkp/linux/commits/Yan-Zhai/packet-add-a-generic-drop-reason-for-receive/20231202-014515">https://github.com/intel-lab-lkp/linux/commits/Yan-Zhai/packet-add-a-generic-drop-reason-for-receive/20231202-014515
s://lore.kernel.org/r/ZWobMUp22oTpP3FW%40debian.debian">https://lore.kernel.org/r/ZWobMUp22oTpP3FW%40debian.debian
v2 net-next] packet: add a generic drop reason for receive
(https://download.01.org/0day-ci/archive/20231206/[email protected]/config)
9.3.0-22) 9.3.0
href="https://download.01.org/0day-ci/archive/20231206/202312061012.2LbT2aSi-lkp@intel.com/reproduce)">https://download.01.org/0day-ci/archive/20231206/[email protected]/reproduce)
in a separate patch/commit (i.e. not just a new version of
kindly add following tags
test robot <[email protected]>
ore.kernel.org/oe-kbuild-all/202312061012.2LbT2aSi-lkp@intel.com/">https://lore.kernel.org/oe-kbuild-all/[email protected]/
prefixed by >>):
In function 'packet_rcv':
warning: variable 'is_drop_n_account' set but not used [-Wunused-but-set-variable]
= false;
/> In function 'tpacket_rcv':
warning: variable 'is_drop_n_account' set but not used [-Wunused-but-set-variable]
= false;
/> +2130 net/packet/af_packet.c
2016-02-03 2108
2005-04-16 2109 /*
2010-12-06 2110 * This function makes lazy skb cloning in hope that most of packets
2010-12-06 2111 * are discarded by BPF.
2010-12-06 2112 *
2010-12-06 2113 * Note tricky part: we DO mangle shared skb! skb->data, skb->len
2010-12-06 2114 * and skb->cb are mangled. It works because (and until) packets
2010-12-06 2115 * falling here are owned by current CPU. Output packets are cloned
2010-12-06 2116 * by dev_queue_xmit_nit(), input packets are processed by net_bh
2021-03-24 2117 * sequentially, so that if we return skb to original state on exit,
2010-12-06 2118 * we will not harm anyone.
2005-04-16 2119 */
2005-04-16 2120
2009-07-21 2121 static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
2009-07-21 2122 struct packet_type *pt, struct net_device *orig_dev)
2005-04-16 2123 {
2005-04-16 2124 struct sock *sk;
2005-04-16 2125 struct sockaddr_ll *sll;
2005-04-16 2126 struct packet_sock *po;
2005-04-16 2127 u8 *skb_head = skb->data;
2005-04-16 2128 int skb_len = skb->len;
2007-01-24 2129 unsigned int snaplen, res;
2016-04-14 @2130 bool is_drop_n_account = false;
2023-12-01 2131 enum skb_drop_reason drop_reason = SKB_CONSUMED;
2005-04-16 2132
2005-04-16 2133 if (skb->pkt_type == PACKET_LOOPBACK)
2005-04-16 2134 goto drop;
2005-04-16 2135
2005-04-16 2136 sk = pt->af_packet_priv;
2005-04-16 2137 po = pkt_sk(sk);
2005-04-16 2138
2009-11-25 2139 if (!net_eq(dev_net(dev), sock_net(sk)))
2007-11-19 2140 goto drop;
2007-11-19 2141
2005-04-16 2142 skb->dev = dev;
2005-04-16 2143
2020-11-21 2144 if (dev_has_header(dev)) {
2005-04-16 2145 /* The device has an explicit notion of ll header,
2010-12-06 2146 * exported to higher levels.
2010-12-06 2147 *
2010-12-06 2148 * Otherwise, the device hides details of its frame
2010-12-06 2149 * structure, so that corresponding packet head is
2010-12-06 2150 * never delivered to user.
2005-04-16 2151 */
2005-04-16 2152 if (sk->sk_type != SOCK_DGRAM)
Carvalho de Melo 2007-03-19 2153 skb_push(skb, skb->data - skb_mac_header(skb));
2005-04-16 2154 else if (skb->pkt_type == PACKET_OUTGOING) {
2005-04-16 2155 /* Special case: outgoing packets have ll header at head */
Carvalho de Melo 2007-03-10 2156 skb_pull(skb, skb_network_offset(skb));
2005-04-16 2157 }
2005-04-16 2158 }
2005-04-16 2159
2005-04-16 2160 snaplen = skb->len;
2005-04-16 2161
2007-01-24 2162 res = run_filter(skb, sk, snaplen);
2007-01-24 2163 if (!res)
2005-04-16 2164 goto drop_n_restore;
2023-12-01 2165
2023-12-01 2166 /* skb will only be "consumed" not "dropped" before this */
2023-12-01 2167 drop_reason = SKB_DROP_REASON_PACKET_SOCK_ERROR;
2023-12-01 2168
2007-01-24 2169 if (snaplen > res)
2007-01-24 2170 snaplen = res;
2005-04-16 2171
2011-12-21 2172 if (atomic_read(&sk->sk_rmem_alloc) >= sk->sk_rcvbuf)
2005-04-16 2173 goto drop_n_acct;
2005-04-16 2174
2005-04-16 2175 if (skb_shared(skb)) {
2005-04-16 2176 struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
2005-04-16 2177 if (nskb == NULL)
2005-04-16 2178 goto drop_n_acct;
2005-04-16 2179
2005-04-16 2180 if (skb_head != skb->data) {
2005-04-16 2181 skb->data = skb_head;
2005-04-16 2182 skb->len = skb_len;
2005-04-16 2183 }
2012-04-19 2184 consume_skb(skb);
2005-04-16 2185 skb = nskb;
2005-04-16 2186 }
2005-04-16 2187
2015-03-01 2188 sock_skb_cb_check_size(sizeof(*PACKET_SKB_CB(skb)) + MAX_ADDR_LEN - 8);
2007-02-04 2189
2007-02-04 2190 sll = &PACKET_SKB_CB(skb)->sa.ll;
2005-04-16 2191 sll->sll_hatype = dev->type;
2005-04-16 2192 sll->sll_pkttype = skb->pkt_type;
2023-03-16 2193 if (unlikely(packet_sock_flag(po, PACKET_SOCK_ORIGDEV)))
2007-04-20 2194 sll->sll_ifindex = orig_dev->ifindex;
2007-04-20 2195 else
2005-04-16 2196 sll->sll_ifindex = dev->ifindex;
2005-04-16 2197
2007-09-26 2198 sll->sll_halen = dev_parse_header(skb, sll->sll_addr);
2005-04-16 2199
2015-03-01 2200 /* sll->sll_family and sll->sll_protocol are set in packet_recvmsg().
2015-03-01 2201 * Use their space for storing the original skb length.
2015-03-01 2202 */
2015-03-01 2203 PACKET_SKB_CB(skb)->sa.origlen = skb->len;
2007-02-04 2204
2005-04-16 2205 if (pskb_trim(skb, snaplen))
2005-04-16 2206 goto drop_n_acct;
2005-04-16 2207
2005-04-16 2208 skb_set_owner_r(skb, sk);
2005-04-16 2209 skb->dev = NULL;
2009-06-02 2210 skb_dst_drop(skb);
2005-04-16 2211
2005-07-12 2212 /* drop conntrack reference */
2019-09-29 2213 nf_reset_ct(skb);
2005-07-12 2214
2005-04-16 2215 spin_lock(&sk->sk_receive_queue.lock);
2013-04-19 2216 po->stats.stats1.tp_packets++;
2015-03-01 2217 sock_skb_set_dropcount(sk, skb);
2022-03-02 2218 skb_clear_delivery_time(skb);
2005-04-16 2219 __skb_queue_tail(&sk->sk_receive_queue, skb);
2005-04-16 2220 spin_unlock(&sk->sk_receive_queue.lock);
2014-04-11 2221 sk->sk_data_ready(sk);
2005-04-16 2222 return 0;
2005-04-16 2223
2005-04-16 2224 drop_n_acct:
2016-04-14 2225 is_drop_n_account = true;
2019-06-12 2226 atomic_inc(&po->tp_drops);
2011-09-30 2227 atomic_inc(&sk->sk_drops);
2005-04-16 2228
2005-04-16 2229 drop_n_restore:
2005-04-16 2230 if (skb_head != skb->data && skb_shared(skb)) {
2005-04-16 2231 skb->data = skb_head;
2005-04-16 2232 skb->len = skb_len;
2005-04-16 2233 }
2005-04-16 2234 drop:
2023-12-01 2235 kfree_skb_reason(skb, drop_reason);
2005-04-16 2236 return 0;
2005-04-16 2237 }
2005-04-16 2238
intel/lkp-tests/wiki">https://github.com/intel/lkp-tests/wiki