From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Lebrun Subject: Re: [PATCH net-next RFC 0/5] ipv6: sr: introduce seg6local End.BPF action Date: Tue, 3 Apr 2018 15:25:31 +0100 Message-ID: References: <20180330230326.ol5f2nmucfjdkop4@ast-mbp.dhcp.thefacebook.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: netdev@vger.kernel.org, David Lebrun , Daniel Borkmann To: Mathieu Xhonneux , Alexei Starovoitov Return-path: Received: from mail-wm0-f43.google.com ([74.125.82.43]:55733 "EHLO mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751095AbeDCOZd (ORCPT ); Tue, 3 Apr 2018 10:25:33 -0400 Received: by mail-wm0-f43.google.com with SMTP id b127so33342804wmf.5 for ; Tue, 03 Apr 2018 07:25:33 -0700 (PDT) In-Reply-To: Content-Language: en-GB Sender: netdev-owner@vger.kernel.org List-ID: On 04/03/2018 02:40 PM, David Lebrun wrote: > On 04/03/2018 12:16 PM, Mathieu Xhonneux wrote: >> >>> In patch 2 I was a bit concerned that: >>> +       struct seg6_bpf_srh_state *srh_state = (struct >>> seg6_bpf_srh_state *) >>> +                                              &skb->cb; >>> would not collide with other users of skb->cb, but it seems the way >>> the hook is placed such usage should always be valid. >>> Would be good to add a comment describing the situation. >> Yes, it's indeed a little hack, but this should be OK since the IPv6 >> layer does >> not use the cb field. Another solution would be to create a new field in >> __sk_buff but it's more cumbersome. >> I will add a comment. > > Good point. The IPv6 layer *does* use the cb field through the IP6CB() > macro. It is first filled in ipv6_rcv() for ingress packets and used, > among others, in the input path by extension headers processing > functions to store EH offsets. > > Given that input_action_end_bpf is called in the forwarding path > and terminates with a call to dst_input(), IP6CB() will be then reset by > ipv6_rcv(), and the use of skb->cb here indeed should not collide with > other users. Actually I'm wrong here. dst_input() will call either ip6_input() or ip6_forward(), not ipv6_rcv(). Both functions expect IP6CB() to be set, so using skb->cb here will interfere with them. What about saving and restoring the IPv6 CB, similarly to what TCP does with tcp_v6_restore_cb() ? David