From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751403AbbCTGqa (ORCPT ); Fri, 20 Mar 2015 02:46:30 -0400 Received: from mail-la0-f43.google.com ([209.85.215.43]:34176 "EHLO mail-la0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750951AbbCTGq3 (ORCPT ); Fri, 20 Mar 2015 02:46:29 -0400 Message-ID: <550BC22F.6090007@gmail.com> Date: Fri, 20 Mar 2015 09:46:07 +0300 From: Alexander Drozdov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Willem de Bruijn CC: "David S. Miller" , Daniel Borkmann , Eric Dumazet , Al Viro , "Michael S. Tsirkin" , Network Development , linux-kernel , tom@herbertland.com Subject: Re: [PATCH 2/2] af_packet: pass checksum validation status to the user References: <1426752102-12786-1-git-send-email-al.drozdov@gmail.com> <1426752102-12786-2-git-send-email-al.drozdov@gmail.com> <550B10A4.60500@gmail.com> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 19.03.2015 21:50:03 +0300 Willem de Bruijn wrote: > On Thu, Mar 19, 2015 at 2:08 PM, Alexander Drozdov wrote: >> On Thu, 19 Mar 2015 11:29:32 -0400, Willem de Bruijn >> wrote: >>> >>> On Thu, Mar 19, 2015 at 4:01 AM, Alexander Drozdov >>> 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 >>>> --- >>>> 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 >>>> >> >>