From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757183AbbCSSIy (ORCPT ); Thu, 19 Mar 2015 14:08:54 -0400 Received: from mail-la0-f53.google.com ([209.85.215.53]:35786 "EHLO mail-la0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753594AbbCSSIv (ORCPT ); Thu, 19 Mar 2015 14:08:51 -0400 Message-ID: <550B10A4.60500@gmail.com> Date: Thu, 19 Mar 2015 21:08:36 +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 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> 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 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, but that is not true for me (in my tests, skb->ip_summed == CHECKSUM_UNNECESSARY for forwarded packets in some cases). > 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. > 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 >>