netdev - Re: [PATCH bpf-next 2/4] bpf: Split bpf_sk

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20181109173034.GA61534@rdna-mbp.dhcp.thefacebook.com>
Date:   Fri, 9 Nov 2018 17:30:36 +0000
From:   Andrey Ignatov <rdna@...com>
To:     Martin Lau <kafai@...com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "joe@...d.net.nz" <joe@...d.net.nz>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 2/4] bpf: Split bpf_sk_lookup

Martin Lau <kafai@...com> [Fri, 2018-11-09 09:19 -0800]:
> On Thu, Nov 08, 2018 at 08:54:23AM -0800, Andrey Ignatov wrote:
> > Split bpf_sk_lookup to separate core functionality, that can be reused
> > to make socket lookup available to more program types, from
> > functionality specific to program types that have access to skb.
> > 
> > Core functionality is placed to __bpf_sk_lookup. And bpf_sk_lookup only
> > gets caller netns and ifindex from skb and passes it to __bpf_sk_lookup.
> > 
> > Program types that don't have access to skb can just pass NULL to
> > __bpf_sk_lookup that will be handled correctly by both inet{,6}_sdif and
> > lookup functions.
> > 
> > This is refactoring that simply moves blocks around and does NOT change
> > existing logic.
> > 
> > Signed-off-by: Andrey Ignatov <rdna@...com>
> > Acked-by: Alexei Starovoitov <ast@...nel.org>
> > ---
> >  net/core/filter.c | 38 +++++++++++++++++++++++---------------
> >  1 file changed, 23 insertions(+), 15 deletions(-)
> > 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 9a1327eb25fa..dc0f86a707b7 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -4825,14 +4825,10 @@ static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
> >  
> >  #ifdef CONFIG_INET
> >  static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
> > -			      struct sk_buff *skb, u8 family, u8 proto)
> > +			      struct sk_buff *skb, u8 family, u8 proto, int dif)
> >  {
> >  	bool refcounted = false;
> >  	struct sock *sk = NULL;
> > -	int dif = 0;
> > -
> > -	if (skb->dev)
> > -		dif = skb->dev->ifindex;
> >  
> >  	if (family == AF_INET) {
> >  		__be32 src4 = tuple->ipv4.saddr;
> > @@ -4875,16 +4871,16 @@ static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
> >  	return sk;
> >  }
> >  
> > -/* bpf_sk_lookup performs the core lookup for different types of sockets,
> > +/* __bpf_sk_lookup performs the core lookup for different types of sockets,
> >   * taking a reference on the socket if it doesn't have the flag SOCK_RCU_FREE.
> >   * Returns the socket as an 'unsigned long' to simplify the casting in the
> >   * callers to satisfy BPF_CALL declarations.
> >   */
> >  static unsigned long
> > -bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> > -	      u8 proto, u64 netns_id, u64 flags)
> > +__bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> > +		u8 proto, u64 netns_id, struct net *caller_net, int ifindex,
> > +		u64 flags)
> That looks a bit different from the one landed to bpf-next.
> You may need to respin the set.

Since Nitin's version is landed now, I'll rebase on top of it and this
patch just won't be needed (initially I did it to unblock myself).

I'll also address the nit in patch 3 and send v2 with both changes.

Thanks Martin!

> >  {
> > -	struct net *caller_net;
> >  	struct sock *sk = NULL;
> >  	u8 family = AF_UNSPEC;
> >  	struct net *net;
> > @@ -4893,19 +4889,15 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> >  	if (unlikely(family == AF_UNSPEC || netns_id > U32_MAX || flags))
> >  		goto out;
> >  
> > -	if (skb->dev)
> > -		caller_net = dev_net(skb->dev);
> > -	else
> > -		caller_net = sock_net(skb->sk);
> >  	if (netns_id) {
> >  		net = get_net_ns_by_id(caller_net, netns_id);
> >  		if (unlikely(!net))
> >  			goto out;
> > -		sk = sk_lookup(net, tuple, skb, family, proto);
> > +		sk = sk_lookup(net, tuple, skb, family, proto, ifindex);
> >  		put_net(net);
> >  	} else {
> >  		net = caller_net;
> > -		sk = sk_lookup(net, tuple, skb, family, proto);
> > +		sk = sk_lookup(net, tuple, skb, family, proto, ifindex);
> >  	}
> >  
> >  	if (sk)
> > @@ -4914,6 +4906,22 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> >  	return (unsigned long) sk;
> >  }
> >  
> > +static unsigned long
> > +bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> > +	      u8 proto, u64 netns_id, u64 flags)
> > +{
> > +	struct net *caller_net = sock_net(skb->sk);
> > +	int ifindex = 0;
> > +
> > +	if (skb->dev) {
> > +		caller_net = dev_net(skb->dev);
> > +		ifindex = skb->dev->ifindex;
> > +	}
> > +
> > +	return __bpf_sk_lookup(skb, tuple, len, proto, netns_id, caller_net,
> > +			       ifindex, flags);
> > +}
> > +
> >  BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
> >  	   struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
> >  {
> > -- 
> > 2.17.1
> > 

-- 
Andrey Ignatov