| 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
| ||
|
Date: Fri, 20 Mar 2015 09:46:07 +0300
From: Alexander Drozdov <al.drozdov@...il.com>
To: Willem de Bruijn <willemb@...gle.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 19.03.2015 21:50:03 +0300 Willem de Bruijn wrote:
> 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?
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@...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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists