Messages in this thread | | | From | Menglong Dong <> | Date | Wed, 16 Mar 2022 14:21:24 +0800 | Subject | Re: [PATCH net-next 3/3] net: ipgre: add skb drop reasons to gre_rcv() |
| |
On Wed, Mar 16, 2022 at 11:17 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 14 Mar 2022 21:33:12 +0800 menglong8.dong@gmail.com wrote: > > From: Menglong Dong <imagedong@tencent.com> > > > > Replace kfree_skb() used in gre_rcv() with kfree_skb_reason(). With > > previous patch, we can tell that no tunnel device is found when > > PACKET_NEXT is returned by erspan_rcv() or ipgre_rcv(). > > > > In this commit, following new drop reasons are added: > > > > SKB_DROP_REASON_GRE_CSUM > > SKB_DROP_REASON_GRE_NOTUNNEL > > > > Reviewed-by: Hao Peng <flyingpeng@tencent.com> > > Reviewed-by: Biao Jiang <benbjiang@tencent.com> > > Signed-off-by: Menglong Dong <imagedong@tencent.com> > > --- > > include/linux/skbuff.h | 2 ++ > > include/trace/events/skb.h | 2 ++ > > net/ipv4/ip_gre.c | 28 ++++++++++++++++++---------- > > 3 files changed, 22 insertions(+), 10 deletions(-) > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index 5edb704af5bb..4f5e58e717ee 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -448,6 +448,8 @@ enum skb_drop_reason { > > SKB_DROP_REASON_GRE_NOHANDLER, /* no handler found (version not > > * supported?) > > */ > > + SKB_DROP_REASON_GRE_CSUM, /* GRE csum error */ > > + SKB_DROP_REASON_GRE_NOTUNNEL, /* no tunnel device found */ > > SKB_DROP_REASON_MAX, > > }; > > > > diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h > > index f2bcffdc4bae..e8f95c96cf9d 100644 > > --- a/include/trace/events/skb.h > > +++ b/include/trace/events/skb.h > > @@ -63,6 +63,8 @@ > > EM(SKB_DROP_REASON_TAP_TXFILTER, TAP_TXFILTER) \ > > EM(SKB_DROP_REASON_GRE_VERSION, GRE_VERSION) \ > > EM(SKB_DROP_REASON_GRE_NOHANDLER, GRE_NOHANDLER) \ > > + EM(SKB_DROP_REASON_GRE_CSUM, GRE_CSUM) \ > > + EM(SKB_DROP_REASON_GRE_NOTUNNEL, GRE_NOTUNNEL) \ > > EMe(SKB_DROP_REASON_MAX, MAX) > > > > #undef EM > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > > index b1579d8374fd..b989239e4abc 100644 > > --- a/net/ipv4/ip_gre.c > > +++ b/net/ipv4/ip_gre.c > > @@ -421,9 +421,10 @@ static int ipgre_rcv(struct sk_buff *skb, const struct tnl_ptk_info *tpi, > > > > static int gre_rcv(struct sk_buff *skb) > > { > > + enum skb_drop_reason reason = SKB_DROP_REASON_NOT_SPECIFIED; > > struct tnl_ptk_info tpi; > > bool csum_err = false; > > - int hdr_len; > > + int hdr_len, ret; > > > > #ifdef CONFIG_NET_IPGRE_BROADCAST > > if (ipv4_is_multicast(ip_hdr(skb)->daddr)) { > > @@ -438,19 +439,26 @@ static int gre_rcv(struct sk_buff *skb) > > I feel like gre_parse_header() is a good candidate for converting > to return a reason instead of errno. >
Enn...isn't gre_parse_header() returning the header length? And I didn't find much useful reason in this function.
> > > goto drop; > > > > if (unlikely(tpi.proto == htons(ETH_P_ERSPAN) || > > - tpi.proto == htons(ETH_P_ERSPAN2))) { > > - if (erspan_rcv(skb, &tpi, hdr_len) == PACKET_RCVD) > > - return 0; > > - goto out; > > - } > > + tpi.proto == htons(ETH_P_ERSPAN2))) > > + ret = erspan_rcv(skb, &tpi, hdr_len); > > + else > > + ret = ipgre_rcv(skb, &tpi, hdr_len); > > ipgre_rcv() OTOH may be better off taking the reason as an output > argument. Assuming PACKET_REJECT means NOMEM is a little fragile.
Yeah, it seems not friendly. I think it's ok to ignore such 'NOMEM' reasons? Therefore, we only need to consider the PACKET_NEXT return value, and keep ipgre_rcv() still.
> > > > > - if (ipgre_rcv(skb, &tpi, hdr_len) == PACKET_RCVD) > > + switch (ret) { > > + case PACKET_NEXT: > > + reason = SKB_DROP_REASON_GRE_NOTUNNEL; > > + break; > > + case PACKET_RCVD: > > return 0; > > - > > -out: > > + case PACKET_REJECT: > > + reason = SKB_DROP_REASON_NOMEM; > > + break; > > + } > > icmp_send(skb, ICMP_DEST_UNREACH, ICMP_PORT_UNREACH, 0); > > drop: > > - kfree_skb(skb); > > + if (csum_err) > > + reason = SKB_DROP_REASON_GRE_CSUM; > > + kfree_skb_reason(skb, reason); > > return 0; > > } > > >
| |