Re: [PATCH net-next 3/3] net: ipgre: add skb drop reasons to gre_rcv()

[Menglong Dong <menglong8.dong@gmail.com>]

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;
> >  }
> >
>