| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+FuTSeJH22uVJcZ6eAxau_ng4LdqLfVSRGYna_pi4pxr1Oz_A@mail.gmail.com>
Date: Thu, 19 Mar 2015 14:50:03 -0400
From: Willem de Bruijn <willemb@...gle.com>
To: Alexander Drozdov <al.drozdov@...il.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Daniel Borkmann <dborkman@...hat.com>,
Eric Dumazet <edumazet@...gle.com>,
Al Viro <viro@...iv.linux.org.uk>,
"Michael S. Tsirkin" <mst@...hat.com>,
Network Development <netdev@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>, tom@...bertland.com
Subject: Re: [PATCH 2/2] af_packet: pass checksum validation status to the user
On Thu, Mar 19, 2015 at 2:08 PM, Alexander Drozdov <al.drozdov@...il.com> wrote:
> On Thu, 19 Mar 2015 11:29:32 -0400, Willem de Bruijn <willemb@...gle.com>
> wrote:
>>
>> On Thu, Mar 19, 2015 at 4:01 AM, Alexander Drozdov <al.drozdov@...il.com>
>> 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?
>> 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.
Please also note the flag and semantics in
Documentation/networking/packet_mmap.txt
>
>
>> Better to support both directions from the start.
>>
>>> Signed-off-by: Alexander Drozdov <al.drozdov@...il.com>
>>> ---
>>> 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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists