Re: [PATCH 2/2] af_packet: pass checksum validation status to the user

[Alexander Drozdov]

On 19.03.2015 21:50:03 +0300 Willem de Bruijn wrote:
> On Thu, Mar 19, 2015 at 2:08 PM, Alexander Drozdov <al.drozdov@xxxxxxxxx> wrote:
> > On Thu, 19 Mar 2015 11:29:32 -0400, Willem de Bruijn <willemb@xxxxxxxxxx>
> > wrote:
> > > On Thu, Mar 19, 2015 at 4:01 AM, Alexander Drozdov <al.drozdov@xxxxxxxxx>
> > > wrote:
> > > > Introduce TP_STATUS_CSUM_VALID tp_status flag to tell the
> > > > af_packet user that at least the transport header checksum
> > > > has been already validated.
> > >
> > > This changes the interface slightly. Processes should be treating
> > > this previously unused bit as reserved and other flags have
> > > been added to the bitmap in this manner as well, so this should
> > > then be safe here, too.
> > >
> > > > For now, the flag may be set for incoming packets only.
> > >
> > > Why?
> >
> > I can't figure out how af_packet could get that the outgoing
> > packet's checksum has been validated. "Checksumming on
> > output" from skbuff.h tells that skb->ip_summed should
> > equal to CHECKSUM_NONE in that case,
>
> CHECKSUM_UNNECESSARY is a valid flag on the outgoing
> path according to that documentation. And, indeed, I also see
> no checksum scrubbing on forwarding paths in practice (but I
> may be wrong there, only took a quick glance).
>
> > but that is not true for me (in my tests, skb->ip_summed ==
> > CHECKSUM_UNNECESSARY for forwarded packets in some
> > cases).
>
> When you disable hardware checksum offload and generate
> packets locally, you do see the expected CHECKSUM_NONE
> value?

Yes, I do, but see below.

> > > You cannot change the semantics of the flag afterwards.
> >
> >
> > I think the semantics of the flag won't be changed if one set the flag
> > for outgoing packets. If the flag is not set (for any directions)
> > then that is not mean that the packet checksum is invalid. The user just can
> > then
> > checksum the packet by itself. So, the user may check the flag for any
> > packet
> > right now.
>
> I see. So it is a hint. Okay. It would be nice if it behaves as
> expected in as many cases as possible from the outset. This
> would include PACKET_OUTGOING and CHECKSUM_NONE.

I've just done some testing, and I've found that packets generated by
'nping --badsum' (socket(PF_INET, SOCK_RAW, IPPROTO_RAW)) have CHECKSUM_NONE
when they are viewed by af_packet. I've used rather old Linux kernel
for the tests, but isn't that a reason to not set TP_STATUS_CSUM_VALID for
outgoing packets right now?

> Please also note the flag and semantics in
> Documentation/networking/packet_mmap.txt

I'll do it and I'll resend the patches with the note.

> > > Better to support both directions from the start.
> > >
> > > > Signed-off-by: Alexander Drozdov <al.drozdov@xxxxxxxxx>
> > > > ---
> > > >    include/uapi/linux/if_packet.h | 1 +
> > > >    net/packet/af_packet.c         | 9 +++++++++
> > > >    2 files changed, 10 insertions(+)
> > > >
> > > > diff --git a/include/uapi/linux/if_packet.h
> > > > b/include/uapi/linux/if_packet.h
> > > > index da2d668..053bd10 100644
> > > > --- a/include/uapi/linux/if_packet.h
> > > > +++ b/include/uapi/linux/if_packet.h
> > > > @@ -99,6 +99,7 @@ struct tpacket_auxdata {
> > > >    #define TP_STATUS_VLAN_VALID           (1 << 4) /* auxdata has valid
> > > > tp_vlan_tci */
> > > >    #define TP_STATUS_BLK_TMO              (1 << 5)
> > > >    #define TP_STATUS_VLAN_TPID_VALID      (1 << 6) /* auxdata has valid
> > > > tp_vlan_tpid */
> > > > +#define TP_STATUS_CSUM_VALID           (1 << 7)
> > > >
> > > >    /* Tx ring - header status */
> > > >    #define TP_STATUS_AVAILABLE          0
> > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > > index 6ecf8dd..3f09dda 100644
> > > > --- a/net/packet/af_packet.c
> > > > +++ b/net/packet/af_packet.c
> > > > @@ -1918,6 +1918,10 @@ static int tpacket_rcv(struct sk_buff *skb, struct
> > > > net_device *dev,
> > > >
> > > >           if (skb->ip_summed == CHECKSUM_PARTIAL)
> > > >                   status |= TP_STATUS_CSUMNOTREADY;
> > > > +       else if (skb->pkt_type != PACKET_OUTGOING &&
> > > > +                (skb->ip_summed == CHECKSUM_COMPLETE ||
> > > > +                 skb_csum_unnecessary(skb)))
> > > > +               status |= TP_STATUS_CSUM_VALID;
> > > >
> > > >           if (snaplen > res)
> > > >                   snaplen = res;
> > > > @@ -3015,6 +3019,11 @@ static int packet_recvmsg(struct kiocb *iocb,
> > > > struct socket *sock,
> > > >                   aux.tp_status = TP_STATUS_USER;
> > > >                   if (skb->ip_summed == CHECKSUM_PARTIAL)
> > > >                           aux.tp_status |= TP_STATUS_CSUMNOTREADY;
> > > > +               else if (skb->pkt_type != PACKET_OUTGOING &&
> > > > +                        (skb->ip_summed == CHECKSUM_COMPLETE ||
> > > > +                         skb_csum_unnecessary(skb)))
> > > > +                       aux.tp_status |= TP_STATUS_CSUM_VALID;
> > > > +
> > >
> > > These two sections are near duplicates. I'd move the entire status
> > > initialization, including existing TP_STATUS_USER and
> > > TP_STATUS_CSUMNOTREADY fields to a helper function.
> > >
> > > It's a bit unfortunately that we have to use an extra bit to add a signal
> > > that is a near inverse of the existing TP_STATUS_CSUMNOTREADY
> > > (bar CHECKSUM_NONE). I do not immediately see a better way,
> > > either, though. And tp_status has plenty room at 32 bits.
> > >
> > > >                   aux.tp_len = PACKET_SKB_CB(skb)->origlen;
> > > >                   aux.tp_snaplen = skb->len;
> > > >                   aux.tp_mac = 0;
> > > > --
> > > > 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/