Netdev List
 help / color / mirror / Atom feed
* Re: [RFC V1 net-next 1/1] net: ena: implement XDP drop support
From: Machulsky, Zorik @ 2019-06-25  3:19 UTC (permalink / raw)
  To: Jesper Dangaard Brouer, Jubran, Samih
  Cc: davem@davemloft.net, netdev@vger.kernel.org, Woodhouse, David,
	Matushevsky, Alexander, Bshara, Saeed, Wilson, Matt,
	Liguori, Anthony, Bshara, Nafea, Tzalik, Guy, Belgazal, Netanel,
	Saidi, Ali, Herrenschmidt, Benjamin, Kiyanovski, Arthur,
	Daniel Borkmann
In-Reply-To: <20190623162133.6b7f24e1@carbon>



On 6/23/19, 7:21 AM, "Jesper Dangaard Brouer" <brouer@redhat.com> wrote:

    On Sun, 23 Jun 2019 10:06:49 +0300 <sameehj@amazon.com> wrote:
    
    > This commit implements the basic functionality of drop/pass logic in the
    > ena driver.
    
    Usually we require a driver to implement all the XDP return codes,
    before we accept it.  But as Daniel and I discussed with Zorik during
    NetConf[1], we are going to make an exception and accept the driver
    if you also implement XDP_TX.
    
    As we trust that Zorik/Amazon will follow and implement XDP_REDIRECT
    later, given he/you wants AF_XDP support which requires XDP_REDIRECT.

Jesper, thanks for your comments and very helpful discussion during NetConf! 
That's the plan, as we agreed. From our side I would like to reiterate again the 
importance of multi-buffer support by xdp frame. We would really prefer not 
to see our MTU shrinking because of xdp support.   

    
    [1] http://vger.kernel.org/netconf2019.html
    -- 
    Best regards,
      Jesper Dangaard Brouer
      MSc.CS, Principal Kernel Engineer at Red Hat
      LinkedIn: http://www.linkedin.com/in/brouer
    


^ permalink raw reply

* Re: Removing skb_orphan() from ip_rcv_core()
From: Joe Stringer @ 2019-06-25  3:17 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Joe Stringer, Eric Dumazet, netdev, john fastabend,
	Daniel Borkmann, Lorenz Bauer, Jakub Sitnicki, Paolo Abeni
In-Reply-To: <20190621205935.og7ajx57j7usgycq@breakpoint.cc>

On Fri, Jun 21, 2019 at 1:59 PM Florian Westphal <fw@strlen.de> wrote:
>
> Joe Stringer <joe@wand.net.nz> wrote:
> > As discussed during LSFMM, I've been looking at adding something like
> > an `skb_sk_assign()` helper to BPF so that logic similar to TPROXY can
> > be implemented with integration into other BPF logic, however
> > currently any attempts to do so are blocked by the skb_orphan() call
> > in ip_rcv_core() (which will effectively ignore any socket assign
> > decision made by the TC BPF program).
> >
> > Recently I was attempting to remove the skb_orphan() call, and I've
> > been trying different things but there seems to be some context I'm
> > missing. Here's the core of the patch:
> >
> > diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
> > index ed97724c5e33..16aea980318a 100644
> > --- a/net/ipv4/ip_input.c
> > +++ b/net/ipv4/ip_input.c
> > @@ -500,8 +500,6 @@ static struct sk_buff *ip_rcv_core(struct sk_buff
> > *skb, struct net *net)
> >        memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
> >        IPCB(skb)->iif = skb->skb_iif;
> >
> > -       /* Must drop socket now because of tproxy. */
> > -       skb_orphan(skb);
> >
> >        return skb;
> >
> > The statement that the socket must be dropped because of tproxy
> > doesn't make sense to me, because the PRE_ROUTING hook is hit after
> > this, which will call into the tproxy logic and eventually
> > nf_tproxy_assign_sock() which already does the skb_orphan() itself.
>
> in comment: s/tproxy/skb_steal_sock/

For reference, I was following the path like this:

ip_rcv()
( -> ip_rcv_core() for skb_orphan)
-> NF_INET_PRE_ROUTING hook
(... invoke iptables hooks)
-> iptable_mangle_hook()
-> ipt_do_table()
... -> tproxy_tg4()
... -> nf_tproxy_assign_sock()
-> skb_orphan()
(... finish iptables processing)
( -> ip_rcv_finish())
( ... -> ip_rcv_finish_core() for early demux / route lookup )
(... -> dst_input())
(... -> tcp_v4_rcv())
( -> __inet_lookup_skb())
( -> skb_steal_sock() )

> at least thats what I concluded a few years ago when I looked into
> the skb_oprhan() need.
>
> IIRC some device drivers use skb->sk for backpressure, so without this
> non-tcp socket would be stolen by skb_steal_sock.

Do you happen to recall which device drivers? Or have some idea of a
list I could try to go through? Are you referring to virtual drivers
like veth or something else?

> We also recently removed skb orphan when crossing netns:
>
> commit 9c4c325252c54b34d53b3d0ffd535182b744e03d
> Author: Flavio Leitner <fbl@redhat.com>
> skbuff: preserve sock reference when scrubbing the skb.
>
> So thats another case where this orphan is needed.

Presumably the orphan is only needed in this case if the packet
crosses a namespace and then is subsequently passed back into the
stack?

> What could be done is adding some way to delay/defer the orphaning
> further, but we would need at the very least some annotation for
> skb_steal_sock to know when the skb->sk is really from TPROXY or
> if it has to orphan.

Eric mentions in another response to this thread that skb_orphan()
should be called from any ndo_start_xmit() which sends traffic back
into the stack. With that, presumably we would be pushing the
orphaning earlier such that the only way that the skb->sk ref can be
non-NULL around this point in receive would be because it was
specifically set by some kind of tproxy logic?

> Same for the safety check in the forwarding path.
> Netfilter modules need o be audited as well, they might make assumptions
> wrt. skb->sk being inet sockets (set by local stack or early demux).
>
> > However, if I drop these lines then I end up causing sockets to
> > release references too many times. Seems like if we don't orphan the
> > skb here, then later logic assumes that we have one more reference
> > than we actually have, and decrements the count when it shouldn't
> > (perhaps the skb_steal_sock() call in __inet_lookup_skb() which seems
> > to assume we always have a reference to the socket?)
>
> We might be calling the wrong destructor (i.e., the one set by tcp
> receive instead of the one set at tx time)?

Hmm, interesting thought. Sure enough, with a bit of bpftrace
debugging we find it's tcp_wfree():

$ cat ip_rcv.bt
#include <linux/skbuff.h>

kprobe:ip_rcv {
       $sk = ((struct sk_buff *)arg0)->sk;
       $des = ((struct sk_buff *)arg0)->destructor;
       if ($sk) {
               if ($des) {
                       printf("received %s on %s with sk destructor %s
set\n", str(arg0), str(arg1), ksym($des));
                       @ip4_stacks[kstack] = count();
               }
       }
}
$ sudo bpftrace ip_rcv.bt
Attaching 1 probe...
received  on eth0 with sk destructor tcp_wfree set
^C

@ip4_stacks[
   ip_rcv+1
   __netif_receive_skb+24
   process_backlog+179
   net_rx_action+304
   __do_softirq+220
   do_softirq_own_stack+42
   do_softirq.part.17+70
   __local_bh_enable_ip+101
   ip_finish_output2+421
   __ip_finish_output+187
   ip_finish_output+44
   ip_output+109
   ip_local_out+59
   __ip_queue_xmit+368
   ip_queue_xmit+16
   __tcp_transmit_skb+1303
   tcp_connect+2758
   tcp_v4_connect+1135
   __inet_stream_connect+214
   inet_stream_connect+59
   __sys_connect+237
   __x64_sys_connect+26
   do_syscall_64+90
   entry_SYSCALL_64_after_hwframe+68
]: 1

Is there a solution here where we call the destructor if it's not
sock_efree()? When the socket is later stolen, it will only return the
reference via a call to sock_put(), so presumably at that point in the
stack we already assume that the skb->destructor is not one of these
other destructors (otherwise we wouldn't release the resources
correctly).

^ permalink raw reply

* [PATCH 2/2] net/ipv6: Fix misuse of proc_dointvec "skip_notify_on_dev_down"
From: Eiichi Tsukata @ 2019-06-25  3:08 UTC (permalink / raw)
  To: gregkh, jslaby, davem, kuznet, yoshfuji, linux-kernel, netdev
  Cc: Eiichi Tsukata
In-Reply-To: <20190625030801.24538-1-devel@etsukata.com>

/proc/sys/net/ipv6/route/skip_notify_on_dev_down assumes given value to be
0 or 1. Use proc_dointvec_minmax instead of proc_dointvec.

Fixes: 7c6bb7d2faaf ("net/ipv6: Add knob to skip DELROUTE message ondevice down")
Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
---
 net/ipv6/route.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 11ad62effd56..aade636c6be6 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -5281,7 +5281,7 @@ static struct ctl_table ipv6_route_table_template[] = {
 		.data		=	&init_net.ipv6.sysctl.skip_notify_on_dev_down,
 		.maxlen		=	sizeof(int),
 		.mode		=	0644,
-		.proc_handler	=	proc_dointvec,
+		.proc_handler	=	proc_dointvec_minmax,
 		.extra1		=	&zero,
 		.extra2		=	&one,
 	},
-- 
2.21.0


^ permalink raw reply related

* [PATCH 1/2] tty: ldisc: Fix misuse of proc_dointvec "ldisc_autoload"
From: Eiichi Tsukata @ 2019-06-25  3:08 UTC (permalink / raw)
  To: gregkh, jslaby, davem, kuznet, yoshfuji, linux-kernel, netdev
  Cc: Eiichi Tsukata

/proc/sys/dev/tty/ldisc_autoload assumes given value to be 0 or 1. Use
proc_dointvec_minmax instead of proc_dointvec.

Fixes: 7c0cca7c847e "(tty: ldisc: add sysctl to prevent autoloading of ldiscs)"
Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
---
 drivers/tty/tty_ldisc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index e38f104db174..a8ea7a35c94e 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -863,7 +863,7 @@ static struct ctl_table tty_table[] = {
 		.data		= &tty_ldisc_autoload,
 		.maxlen		= sizeof(tty_ldisc_autoload),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &zero,
 		.extra2		= &one,
 	},
-- 
2.21.0


^ permalink raw reply related

* [PATCH net-next] xdp: Make __mem_id_disconnect static
From: YueHaibing @ 2019-06-25  2:31 UTC (permalink / raw)
  To: davem, ast, daniel, jakub.kicinski, hawk, john.fastabend
  Cc: linux-kernel, netdev, xdp-newbies, bpf, YueHaibing

Fix sparse warning:

net/core/xdp.c:88:6: warning:
 symbol '__mem_id_disconnect' was not declared. Should it be static?

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 net/core/xdp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/xdp.c b/net/core/xdp.c
index b29d7b5..829377c 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -85,7 +85,7 @@ static void __xdp_mem_allocator_rcu_free(struct rcu_head *rcu)
 	kfree(xa);
 }
 
-bool __mem_id_disconnect(int id, bool force)
+static bool __mem_id_disconnect(int id, bool force)
 {
 	struct xdp_mem_allocator *xa;
 	bool safe_to_remove = true;
-- 
2.7.4



^ permalink raw reply related

* Re: [PATCH net-next] net: link_watch: prevent starvation when processing linkwatch wq
From: Yunsheng Lin @ 2019-06-25  2:28 UTC (permalink / raw)
  To: David Miller
  Cc: hkallweit1, f.fainelli, netdev, linux-kernel, linuxarm, pbonzini,
	rkrcmar, kvm, xuwei (O)
In-Reply-To: <6e9b41c9-6edb-be7f-07ee-5480162a227e@huawei.com>

On 2019/5/29 16:59, Yunsheng Lin wrote:
> On 2019/5/29 14:58, David Miller wrote:
>> From: Yunsheng Lin <linyunsheng@huawei.com>
>> Date: Mon, 27 May 2019 09:47:54 +0800
>>
>>> When user has configured a large number of virtual netdev, such
>>> as 4K vlans, the carrier on/off operation of the real netdev
>>> will also cause it's virtual netdev's link state to be processed
>>> in linkwatch. Currently, the processing is done in a work queue,
>>> which may cause worker starvation problem for other work queue.
>>>
>>> This patch releases the cpu when link watch worker has processed
>>> a fixed number of netdev' link watch event, and schedule the
>>> work queue again when there is still link watch event remaining.
>>>
>>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>>
>> Why not rtnl_unlock(); yield(); rtnl_lock(); every "100" events
>> processed?
>>
>> That seems better than adding all of this overhead to reschedule the
>> workqueue every 100 items.
> 
> One minor concern, the above solution does not seem to solve the cpu
> starvation for other normal workqueue which was scheduled on the same
> cpu as linkwatch. Maybe I misunderstand the workqueue or there is other
> consideration here? :)
> 
> Anyway, I will implemet it as you suggested and test it before posting V2.
> Thanks.

Hi, David

I stress tested the above solution with a lot of vlan dev and qemu-kvm with
vf passthrongh mode, the linkwatch wq sometimes block the irqfd_inject wq
when they are scheduled on the same cpu, which may cause interrupt delay
problem for vm.

Rescheduling workqueue every 100 items does give irqfd_inject wq to run sooner,
which alleviate the interrupt delay problems for vm.

So It is ok for me to fall back to reschedule the link watch wq every 100 items,
or is there a better way to fix it properly?



> 
>>
>> .
>>
> 
> 
> .
> 


^ permalink raw reply

* Re: [PATCH v2 0/3] fix bugs when enable route_localnet
From: Luoshijie (Poincare Lab) @ 2019-06-25  1:50 UTC (permalink / raw)
  To: David Miller
  Cc: tgraf, dsahern, netdev, liuzhiqiang26, wangxiaogang3, mingfangsen,
	zhoukang7
In-Reply-To: <20190624.090325.163853495970223718.davem@davemloft.net>

On 2019/6/25 0:03, David Miller wrote:
> From: luoshijie <luoshijie1@huawei.com>
> Date: Tue, 18 Jun 2019 15:14:02 +0000
> 
>> From: Shijie Luo <luoshijie1@huawei.com>
>>
>> When enable route_localnet, route of the 127/8 address is enabled.
>> But in some situations like arp_announce=2, ARP requests or reply
>> work abnormally.
>>
>> This patchset fix some bugs when enable route_localnet. 
>>
>> Change History:
>> V2:
>> - Change a single patch to a patchset.
>> - Add bug fix for arp_ignore = 3.
>> - Add a couple of test for enabling route_localnet in selftests.
> 
> Series applied to net-next, thanks.
> 

Thanks a lot, and I'm truly grateful for advice and help of David Ahern.


^ permalink raw reply

* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Takshak Chahande @ 2019-06-25  1:39 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Jakub Kicinski, Andrey Ignatov, Daniel Borkmann,
	netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
	Stanislav Fomichev
In-Reply-To: <99a92dd0-4914-aeb5-709b-ed4615820aa0@fb.com>

Alexei Starovoitov <ast@fb.com> wrote on Mon [2019-Jun-24 18:23:28 -0700]:
> On 6/24/19 5:57 PM, Jakub Kicinski wrote:
> > On Mon, 24 Jun 2019 17:47:26 -0700, Jakub Kicinski wrote:
> >> I see.  The local flag would not an option in getopt_long() sense, what
> >> I was thinking was about adding an "effective" keyword:
> > 
> > Something like this, untested:
> > 
> > --->8------------
> > 
> > The BPF_F_QUERY_EFFECTIVE is a syscall flag, and fits nicely
> > as a subcommand option.  We want to move away from global
> > options, anyway.
> > 
> > We need a global variable because of nftw limitations.
> > Clean this flag on every invocations in case we run in
> > batch mode.
> > 
> > NOTE the argv[1] use on the error path in do_show() looks
> > like a bug on it's own.
> > ---
> >   .../bpftool/Documentation/bpftool-cgroup.rst  | 24 +++----
> >   tools/bpf/bpftool/Documentation/bpftool.rst   |  6 +-
> >   tools/bpf/bpftool/bash-completion/bpftool     | 17 ++---
> >   tools/bpf/bpftool/cgroup.c                    | 62 ++++++++++++-------
> >   tools/bpf/bpftool/main.c                      |  7 +--
> >   tools/bpf/bpftool/main.h                      |  3 +-
> >   6 files changed, 66 insertions(+), 53 deletions(-)
> > 
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> > index 324df15bf4cc..4fde3dfad395 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> > @@ -12,8 +12,7 @@ SYNOPSIS
> >   
> >   	**bpftool** [*OPTIONS*] **cgroup** *COMMAND*
> >   
> > -	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** }
> > -	| { **-e** | **--effective** } }
> > +	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
> >   
> >   	*COMMANDS* :=
> >   	{ **show** | **list** | **tree** | **attach** | **detach** | **help** }
> > @@ -21,8 +20,8 @@ SYNOPSIS
> >   CGROUP COMMANDS
> >   ===============
> >   
> > -|	**bpftool** **cgroup { show | list }** *CGROUP*
> > -|	**bpftool** **cgroup tree** [*CGROUP_ROOT*]
> > +|	**bpftool** **cgroup { show | list }** *CGROUP* [**effective**]
> > +|	**bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**]
> >   |	**bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> >   |	**bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
> >   |	**bpftool** **cgroup help**
> > @@ -35,13 +34,17 @@ CGROUP COMMANDS
> >   
> >   DESCRIPTION
> >   ===========
> > -	**bpftool cgroup { show | list }** *CGROUP*
> > +	**bpftool cgroup { show | list }** *CGROUP* [**effective**]
> >   		  List all programs attached to the cgroup *CGROUP*.
> >   
> >   		  Output will start with program ID followed by attach type,
> >   		  attach flags and program name.
> >   
> > -	**bpftool cgroup tree** [*CGROUP_ROOT*]
> > +		  If **effective** is specified retrieve effective programs that
> > +		  will execute for events within a cgroup. This includes
> > +		  inherited along with attached ones.
> > +
> > +	**bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**]
> >   		  Iterate over all cgroups in *CGROUP_ROOT* and list all
> >   		  attached programs. If *CGROUP_ROOT* is not specified,
> >   		  bpftool uses cgroup v2 mountpoint.
> > @@ -50,6 +53,10 @@ DESCRIPTION
> >   		  commands: it starts with absolute cgroup path, followed by
> >   		  program ID, attach type, attach flags and program name.
> >   
> > +		  If **effective** is specified retrieve effective programs that
> > +		  will execute for events within a cgroup. This includes
> > +		  inherited along with attached ones.
> > +
> >   	**bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> >   		  Attach program *PROG* to the cgroup *CGROUP* with attach type
> >   		  *ATTACH_TYPE* and optional *ATTACH_FLAGS*.
> > @@ -122,11 +129,6 @@ OPTIONS
> >   		  Print all logs available from libbpf, including debug-level
> >   		  information.
> >   
> > -	-e, --effective
> > -		  Retrieve effective programs that will execute for events
> > -		  within a cgroup. This includes inherited along with attached
> > -		  ones.
> > -
> >   EXAMPLES
> >   ========
> >   |
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
> > index d2f76b55988d..6a9c52ef84a9 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool.rst
> > @@ -19,7 +19,7 @@ SYNOPSIS
> >   	*OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** | **feature** }
> >   
> >   	*OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** }
> > -	| { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** | **--effective** } }
> > +	| { **-j** | **--json** } [{ **-p** | **--pretty** }] }
> >   
> >   	*MAP-COMMANDS* :=
> >   	{ **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext**
> > @@ -71,10 +71,6 @@ OPTIONS
> >   		  includes logs from libbpf as well as from the verifier, when
> >   		  attempting to load programs.
> >   
> > -	-e, --effective
> > -		  Retrieve effective programs that will execute for events
> > -		  within a cgroup. This includes inherited along with attached ones.
> > -
> >   SEE ALSO
> >   ========
> >   	**bpf**\ (2),
> > diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> > index c98cb99867f6..de84ae06ae4e 100644
> > --- a/tools/bpf/bpftool/bash-completion/bpftool
> > +++ b/tools/bpf/bpftool/bash-completion/bpftool
> > @@ -187,7 +187,7 @@ _bpftool()
> >   
> >       # Deal with options
> >       if [[ ${words[cword]} == -* ]]; then
> > -        local c='--version --json --pretty --bpffs --mapcompat --debug --effective'
> > +        local c='--version --json --pretty --bpffs --mapcompat --debug'
> >           COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
> >           return 0
> >       fi
> > @@ -678,12 +678,15 @@ _bpftool()
> >               ;;
> >           cgroup)
> >               case $command in
> > -                show|list)
> > -                    _filedir
> > -                    return 0
> > -                    ;;
> > -                tree)
> > -                    _filedir
> > +                show|list|tree)
> > +                    case $cword in
> > +                        3)
> > +                            _filedir
> > +                            ;;
> > +                        4)
> > +                            COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) )
> > +                            ;;
> > +                    esac
> >                       return 0
> >                       ;;
> >                   attach|detach)
> > diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> > index 1bb2a751107a..88b80616d47b 100644
> > --- a/tools/bpf/bpftool/cgroup.c
> > +++ b/tools/bpf/bpftool/cgroup.c
> > @@ -28,6 +28,8 @@
> >   	"                        connect6 | sendmsg4 | sendmsg6 |\n"           \
> >   	"                        recvmsg4 | recvmsg6 | sysctl }"
> >   
> > +static unsigned int query_flags;
> > +
> >   static const char * const attach_type_strings[] = {
> >   	[BPF_CGROUP_INET_INGRESS] = "ingress",
> >   	[BPF_CGROUP_INET_EGRESS] = "egress",
> > @@ -104,8 +106,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
> >   	__u32 prog_cnt = 0;
> >   	int ret;
> >   
> > -	ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL,
> > -			     &prog_cnt);
> > +	ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL,
> > +			     NULL, &prog_cnt);
> >   	if (ret)
> >   		return -1;
> >   
> > @@ -156,20 +158,30 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
> >   static int do_show(int argc, char **argv)
> >   {
> >   	enum bpf_attach_type type;
> > +	const char *path;
> >   	int cgroup_fd;
> >   	int ret = -1;
> >   
> > -	if (argc < 1) {
> > -		p_err("too few parameters for cgroup show");
> > -		goto exit;
> > -	} else if (argc > 1) {
> > -		p_err("too many parameters for cgroup show");
> > -		goto exit;
> > +	query_flags = 0;
> > +
> > +	if (!REQ_ARGS(1))
> > +		return -1;
> > +	path = GET_ARG();
> > +
> > +	while (argc) {
> > +		if (is_prefix(*argv, "effective")) {
> > +			query_flags |= BPF_F_QUERY_EFFECTIVE;
> > +			NEXT_ARG();
> > +		} else {
> > +			p_err("expected no more arguments, 'effective', got: '%s'?",
> > +			      *argv);
> > +			return -1;
> > +		}
> >   	}
> >   
> > -	cgroup_fd = open(argv[0], O_RDONLY);
> > +	cgroup_fd = open(path, O_RDONLY);
> >   	if (cgroup_fd < 0) {
> > -		p_err("can't open cgroup %s", argv[1]);
> > +		p_err("can't open cgroup %s", path);
> >   		goto exit;
> >   	}
> >   
> > @@ -295,23 +307,29 @@ static int do_show_tree(int argc, char **argv)
> >   	char *cgroup_root;
> >   	int ret;
> >   
> > -	switch (argc) {
> > -	case 0:
> > +	query_flags = 0;
> > +
> > +	if (!argc) {
> >   		cgroup_root = find_cgroup_root();
> >   		if (!cgroup_root) {
> >   			p_err("cgroup v2 isn't mounted");
> >   			return -1;
> >   		}
> > -		break;
> > -	case 1:
> > -		cgroup_root = argv[0];
> > -		break;
> > -	default:
> > -		p_err("too many parameters for cgroup tree");
> > -		return -1;
> > +	} else {
> > +		cgroup_root = GET_ARG();
> > +
> > +		while (argc) {
> > +			if (is_prefix(*argv, "effective")) {
> > +				query_flags |= BPF_F_QUERY_EFFECTIVE;
> > +				NEXT_ARG();
> > +			} else {
> > +				p_err("expected no more arguments, 'effective', got: '%s'?",
> > +				      *argv);
> > +				return -1;
> > +			}
> > +		}
> >   	}
> >   
> > -
> >   	if (json_output)
> >   		jsonw_start_array(json_wtr);
> >   	else
> > @@ -457,8 +475,8 @@ static int do_help(int argc, char **argv)
> >   	}
> >   
> >   	fprintf(stderr,
> > -		"Usage: %s %s { show | list } CGROUP\n"
> > -		"       %s %s tree [CGROUP_ROOT]\n"
> > +		"Usage: %s %s { show | list } CGROUP [**effective**]\n"
> > +		"       %s %s tree [CGROUP_ROOT] [**effective**]\n"
> 
> lgtm.
> Takshak, Andrey, wdyt?
ya, looks fine to me.

--Takshak

^ permalink raw reply

* Re: [PATCH v2 0/3] fix bugs when enable route_localnet
From: Zhiqiang Liu @ 2019-06-25  1:25 UTC (permalink / raw)
  To: David Miller, luoshijie1
  Cc: tgraf, dsahern, netdev, wangxiaogang3, mingfangsen, zhoukang7
In-Reply-To: <20190624.090325.163853495970223718.davem@davemloft.net>



On 2019/6/25 0:03, David Miller wrote:
> From: luoshijie <luoshijie1@huawei.com>
> Date: Tue, 18 Jun 2019 15:14:02 +0000
> 
>> From: Shijie Luo <luoshijie1@huawei.com>
>>
>> When enable route_localnet, route of the 127/8 address is enabled.
>> But in some situations like arp_announce=2, ARP requests or reply
>> work abnormally.
>>
>> This patchset fix some bugs when enable route_localnet. 
>>
>> Change History:
>> V2:
>> - Change a single patch to a patchset.
>> - Add bug fix for arp_ignore = 3.
>> - Add a couple of test for enabling route_localnet in selftests.
> 
> Series applied to net-next, thanks.
> 
Thanks again for you and David Ahern.


^ permalink raw reply

* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Alexei Starovoitov @ 2019-06-25  1:23 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
	netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
	Stanislav Fomichev
In-Reply-To: <20190624175740.5cccea9b@cakuba.netronome.com>

On 6/24/19 5:57 PM, Jakub Kicinski wrote:
> On Mon, 24 Jun 2019 17:47:26 -0700, Jakub Kicinski wrote:
>> I see.  The local flag would not an option in getopt_long() sense, what
>> I was thinking was about adding an "effective" keyword:
> 
> Something like this, untested:
> 
> --->8------------
> 
> The BPF_F_QUERY_EFFECTIVE is a syscall flag, and fits nicely
> as a subcommand option.  We want to move away from global
> options, anyway.
> 
> We need a global variable because of nftw limitations.
> Clean this flag on every invocations in case we run in
> batch mode.
> 
> NOTE the argv[1] use on the error path in do_show() looks
> like a bug on it's own.
> ---
>   .../bpftool/Documentation/bpftool-cgroup.rst  | 24 +++----
>   tools/bpf/bpftool/Documentation/bpftool.rst   |  6 +-
>   tools/bpf/bpftool/bash-completion/bpftool     | 17 ++---
>   tools/bpf/bpftool/cgroup.c                    | 62 ++++++++++++-------
>   tools/bpf/bpftool/main.c                      |  7 +--
>   tools/bpf/bpftool/main.h                      |  3 +-
>   6 files changed, 66 insertions(+), 53 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> index 324df15bf4cc..4fde3dfad395 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> @@ -12,8 +12,7 @@ SYNOPSIS
>   
>   	**bpftool** [*OPTIONS*] **cgroup** *COMMAND*
>   
> -	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** }
> -	| { **-e** | **--effective** } }
> +	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
>   
>   	*COMMANDS* :=
>   	{ **show** | **list** | **tree** | **attach** | **detach** | **help** }
> @@ -21,8 +20,8 @@ SYNOPSIS
>   CGROUP COMMANDS
>   ===============
>   
> -|	**bpftool** **cgroup { show | list }** *CGROUP*
> -|	**bpftool** **cgroup tree** [*CGROUP_ROOT*]
> +|	**bpftool** **cgroup { show | list }** *CGROUP* [**effective**]
> +|	**bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**]
>   |	**bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
>   |	**bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
>   |	**bpftool** **cgroup help**
> @@ -35,13 +34,17 @@ CGROUP COMMANDS
>   
>   DESCRIPTION
>   ===========
> -	**bpftool cgroup { show | list }** *CGROUP*
> +	**bpftool cgroup { show | list }** *CGROUP* [**effective**]
>   		  List all programs attached to the cgroup *CGROUP*.
>   
>   		  Output will start with program ID followed by attach type,
>   		  attach flags and program name.
>   
> -	**bpftool cgroup tree** [*CGROUP_ROOT*]
> +		  If **effective** is specified retrieve effective programs that
> +		  will execute for events within a cgroup. This includes
> +		  inherited along with attached ones.
> +
> +	**bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**]
>   		  Iterate over all cgroups in *CGROUP_ROOT* and list all
>   		  attached programs. If *CGROUP_ROOT* is not specified,
>   		  bpftool uses cgroup v2 mountpoint.
> @@ -50,6 +53,10 @@ DESCRIPTION
>   		  commands: it starts with absolute cgroup path, followed by
>   		  program ID, attach type, attach flags and program name.
>   
> +		  If **effective** is specified retrieve effective programs that
> +		  will execute for events within a cgroup. This includes
> +		  inherited along with attached ones.
> +
>   	**bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
>   		  Attach program *PROG* to the cgroup *CGROUP* with attach type
>   		  *ATTACH_TYPE* and optional *ATTACH_FLAGS*.
> @@ -122,11 +129,6 @@ OPTIONS
>   		  Print all logs available from libbpf, including debug-level
>   		  information.
>   
> -	-e, --effective
> -		  Retrieve effective programs that will execute for events
> -		  within a cgroup. This includes inherited along with attached
> -		  ones.
> -
>   EXAMPLES
>   ========
>   |
> diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
> index d2f76b55988d..6a9c52ef84a9 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool.rst
> @@ -19,7 +19,7 @@ SYNOPSIS
>   	*OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** | **feature** }
>   
>   	*OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** }
> -	| { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** | **--effective** } }
> +	| { **-j** | **--json** } [{ **-p** | **--pretty** }] }
>   
>   	*MAP-COMMANDS* :=
>   	{ **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext**
> @@ -71,10 +71,6 @@ OPTIONS
>   		  includes logs from libbpf as well as from the verifier, when
>   		  attempting to load programs.
>   
> -	-e, --effective
> -		  Retrieve effective programs that will execute for events
> -		  within a cgroup. This includes inherited along with attached ones.
> -
>   SEE ALSO
>   ========
>   	**bpf**\ (2),
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index c98cb99867f6..de84ae06ae4e 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -187,7 +187,7 @@ _bpftool()
>   
>       # Deal with options
>       if [[ ${words[cword]} == -* ]]; then
> -        local c='--version --json --pretty --bpffs --mapcompat --debug --effective'
> +        local c='--version --json --pretty --bpffs --mapcompat --debug'
>           COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
>           return 0
>       fi
> @@ -678,12 +678,15 @@ _bpftool()
>               ;;
>           cgroup)
>               case $command in
> -                show|list)
> -                    _filedir
> -                    return 0
> -                    ;;
> -                tree)
> -                    _filedir
> +                show|list|tree)
> +                    case $cword in
> +                        3)
> +                            _filedir
> +                            ;;
> +                        4)
> +                            COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) )
> +                            ;;
> +                    esac
>                       return 0
>                       ;;
>                   attach|detach)
> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> index 1bb2a751107a..88b80616d47b 100644
> --- a/tools/bpf/bpftool/cgroup.c
> +++ b/tools/bpf/bpftool/cgroup.c
> @@ -28,6 +28,8 @@
>   	"                        connect6 | sendmsg4 | sendmsg6 |\n"           \
>   	"                        recvmsg4 | recvmsg6 | sysctl }"
>   
> +static unsigned int query_flags;
> +
>   static const char * const attach_type_strings[] = {
>   	[BPF_CGROUP_INET_INGRESS] = "ingress",
>   	[BPF_CGROUP_INET_EGRESS] = "egress",
> @@ -104,8 +106,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
>   	__u32 prog_cnt = 0;
>   	int ret;
>   
> -	ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL,
> -			     &prog_cnt);
> +	ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL,
> +			     NULL, &prog_cnt);
>   	if (ret)
>   		return -1;
>   
> @@ -156,20 +158,30 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
>   static int do_show(int argc, char **argv)
>   {
>   	enum bpf_attach_type type;
> +	const char *path;
>   	int cgroup_fd;
>   	int ret = -1;
>   
> -	if (argc < 1) {
> -		p_err("too few parameters for cgroup show");
> -		goto exit;
> -	} else if (argc > 1) {
> -		p_err("too many parameters for cgroup show");
> -		goto exit;
> +	query_flags = 0;
> +
> +	if (!REQ_ARGS(1))
> +		return -1;
> +	path = GET_ARG();
> +
> +	while (argc) {
> +		if (is_prefix(*argv, "effective")) {
> +			query_flags |= BPF_F_QUERY_EFFECTIVE;
> +			NEXT_ARG();
> +		} else {
> +			p_err("expected no more arguments, 'effective', got: '%s'?",
> +			      *argv);
> +			return -1;
> +		}
>   	}
>   
> -	cgroup_fd = open(argv[0], O_RDONLY);
> +	cgroup_fd = open(path, O_RDONLY);
>   	if (cgroup_fd < 0) {
> -		p_err("can't open cgroup %s", argv[1]);
> +		p_err("can't open cgroup %s", path);
>   		goto exit;
>   	}
>   
> @@ -295,23 +307,29 @@ static int do_show_tree(int argc, char **argv)
>   	char *cgroup_root;
>   	int ret;
>   
> -	switch (argc) {
> -	case 0:
> +	query_flags = 0;
> +
> +	if (!argc) {
>   		cgroup_root = find_cgroup_root();
>   		if (!cgroup_root) {
>   			p_err("cgroup v2 isn't mounted");
>   			return -1;
>   		}
> -		break;
> -	case 1:
> -		cgroup_root = argv[0];
> -		break;
> -	default:
> -		p_err("too many parameters for cgroup tree");
> -		return -1;
> +	} else {
> +		cgroup_root = GET_ARG();
> +
> +		while (argc) {
> +			if (is_prefix(*argv, "effective")) {
> +				query_flags |= BPF_F_QUERY_EFFECTIVE;
> +				NEXT_ARG();
> +			} else {
> +				p_err("expected no more arguments, 'effective', got: '%s'?",
> +				      *argv);
> +				return -1;
> +			}
> +		}
>   	}
>   
> -
>   	if (json_output)
>   		jsonw_start_array(json_wtr);
>   	else
> @@ -457,8 +475,8 @@ static int do_help(int argc, char **argv)
>   	}
>   
>   	fprintf(stderr,
> -		"Usage: %s %s { show | list } CGROUP\n"
> -		"       %s %s tree [CGROUP_ROOT]\n"
> +		"Usage: %s %s { show | list } CGROUP [**effective**]\n"
> +		"       %s %s tree [CGROUP_ROOT] [**effective**]\n"

lgtm.
Takshak, Andrey, wdyt?

^ permalink raw reply

* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Alexei Starovoitov @ 2019-06-25  1:22 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
	netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
	Stanislav Fomichev
In-Reply-To: <20190624174726.2dda122b@cakuba.netronome.com>

On 6/24/19 5:47 PM, Jakub Kicinski wrote:
> On Tue, 25 Jun 2019 00:40:09 +0000, Alexei Starovoitov wrote:
>> On 6/24/19 5:30 PM, Jakub Kicinski wrote:
>>> On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote:
>>>> On 6/24/19 5:16 PM, Jakub Kicinski wrote:
>>>>> On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote:
>>>>>> I don't think this patch should be penalized.
>>>>>> I'd rather see we fix them all.
>>>>>
>>>>> So we are going to add this broken option just to remove it?
>>>>> I don't understand.
>>>>> I'm happy to spend the 15 minutes rewriting this if you don't
>>>>> want to penalize Takshak.
>>>>
>>>> hmm. I don't understand the 'broken' part.
>>>> The only issue I see that it could have been local vs global,
>>>> but they all should have been local.
>>>
>>> I don't think all of them.  Only --mapcompat and --bpffs.  bpffs could
>>> be argued.  On mapcompat I must have not read the patch fully, I was
>>> under the impression its a global libbpf flag :(
>>>
>>> --json, --pretty, --nomount, --debug are global because they affect
>>> global behaviour of bpftool.  The difference here is that we basically
>>> add a syscall parameter as a global option.
>>
>> sure. I only disagreed about not touching older flags.
>> --effective should be local.
>> If follow up patch means 90% rewrite then revert is better.
>> If it's 10% fixup then it's different story.
> 
> I see.  The local flag would not an option in getopt_long() sense, what
> I was thinking was about adding an "effective" keyword:
> 
> # bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/
> 
> becomes:
> 
> # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ effective
> 
> That's how we handle flags for update calls for instance..
> 
> So I think a revert :(

fair enough.
removed it and force pushed bpf-next.


^ permalink raw reply

* [PATCH net] ipv4: Use return value of inet_iif() for __raw_v4_lookup in the while loop
From: Stephen Suryaputra @ 2019-06-25  0:14 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, Stephen Suryaputra

In commit 19e4e768064a8 ("ipv4: Fix raw socket lookup for local
traffic"), the dif argument to __raw_v4_lookup() is coming from the
returned value of inet_iif() but the change was done only for the first
lookup. Subsequent lookups in the while loop still use skb->dev->ifIndex.

Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
---
 net/ipv4/raw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index 0b8e06ca75d6..40a6abbc9cf6 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -197,7 +197,7 @@ static int raw_v4_input(struct sk_buff *skb, const struct iphdr *iph, int hash)
 		}
 		sk = __raw_v4_lookup(net, sk_next(sk), iph->protocol,
 				     iph->saddr, iph->daddr,
-				     skb->dev->ifindex, sdif);
+				     dif, sdif);
 	}
 out:
 	read_unlock(&raw_v4_hashinfo.lock);
-- 
2.17.1


^ permalink raw reply related

* [PATCH net-next 1/1] tc-testing:  Restore original behaviour for namespaces in tdc
From: Lucas Bates @ 2019-06-25  1:00 UTC (permalink / raw)
  To: davem
  Cc: netdev, nicolas.dichtel, jhs, xiyou.wangcong, jiri, mleitner,
	vladbu, dcaratti, kernel, Lucas Bates

This patch restores the original behaviour for tdc prior to the
introduction of the plugin system, where the network namespace
functionality was split from the main script.

It introduces the concept of required plugins for testcases,
and will automatically load any plugin that isn't already
enabled when said plugin is required by even one testcase.

Additionally, the -n option for the nsPlugin is deprecated
so the default action is to make use of the namespaces.
Instead, we introduce -N to not use them, but still create
the veth pair.

buildebpfPlugin's -B option is also deprecated.

If a test cases requires the features of a specific plugin
in order to pass, it should instead include a new key/value
pair describing plugin interactions:

        "plugins": {
                "requires": "buildebpfPlugin"
        },

A test case can have more than one required plugin: a list
can be inserted as the value for 'requires'.

Signed-off-by: Lucas Bates <lucasb@mojatatu.com>
---
 tools/testing/selftests/tc-testing/README          |  22 ++-
 .../tc-testing/plugin-lib/buildebpfPlugin.py       |   5 +-
 .../selftests/tc-testing/plugin-lib/nsPlugin.py    |  26 +++-
 .../selftests/tc-testing/tc-tests/actions/bpf.json |   6 +
 .../selftests/tc-testing/tc-tests/filters/fw.json  | 162 +++++++++++++++++++++
 .../tc-testing/tc-tests/filters/tests.json         |  12 ++
 tools/testing/selftests/tc-testing/tdc.py          |  78 +++++++++-
 tools/testing/selftests/tc-testing/tdc_helper.py   |   5 +-
 8 files changed, 296 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/tc-testing/README b/tools/testing/selftests/tc-testing/README
index f9281e8..22e5da9 100644
--- a/tools/testing/selftests/tc-testing/README
+++ b/tools/testing/selftests/tc-testing/README
@@ -12,10 +12,10 @@ REQUIREMENTS
 *  Minimum Python version of 3.4. Earlier 3.X versions may work but are not
    guaranteed.
 
-*  The kernel must have network namespace support
+*  The kernel must have network namespace support if using nsPlugin
 
 *  The kernel must have veth support available, as a veth pair is created
-   prior to running the tests.
+   prior to running the tests when using nsPlugin.
 
 *  The kernel must have the appropriate infrastructure enabled to run all tdc
    unit tests. See the config file in this directory for minimum required
@@ -53,8 +53,12 @@ commands being tested must be run as root.  The code that enforces
 execution by root uid has been moved into a plugin (see PLUGIN
 ARCHITECTURE, below).
 
-If nsPlugin is linked, all tests are executed inside a network
-namespace to prevent conflicts within the host.
+Tests that use a network device should have nsPlugin.py listed as a
+requirement for that test. nsPlugin executes all commands within a
+network namespace and creates a veth pair which may be used in those test
+cases. To disable execution within the namespace, pass the -N option
+to tdc when starting a test run; the veth pair will still be created
+by the plugin.
 
 Running tdc without any arguments will run all tests. Refer to the section
 on command line arguments for more information, or run:
@@ -154,8 +158,8 @@ action:
 netns:
   options for nsPlugin (run commands in net namespace)
 
-  -n, --namespace
-                        Run commands in namespace as specified in tdc_config.py
+  -N, --no-namespace
+                        Do not run commands in a network namespace.
 
 valgrind:
   options for valgrindPlugin (run command under test under Valgrind)
@@ -171,7 +175,8 @@ was in the tdc.py script has been moved into the plugins.
 
 The plugins are in the directory plugin-lib.  The are executed from
 directory plugins.  Put symbolic links from plugins to plugin-lib,
-and name them according to the order you want them to run.
+and name them according to the order you want them to run. This is not
+necessary if a test case being run requires a specific plugin to work.
 
 Example:
 
@@ -223,7 +228,8 @@ directory:
   - rootPlugin.py:
       implements the enforcement of running as root
   - nsPlugin.py:
-      sets up a network namespace and runs all commands in that namespace
+      sets up a network namespace and runs all commands in that namespace,
+      while also setting up dummy devices to be used in testing.
   - valgrindPlugin.py
       runs each command in the execute stage under valgrind,
       and checks for leaks.
diff --git a/tools/testing/selftests/tc-testing/plugin-lib/buildebpfPlugin.py b/tools/testing/selftests/tc-testing/plugin-lib/buildebpfPlugin.py
index 9f0ba10..e98c367 100644
--- a/tools/testing/selftests/tc-testing/plugin-lib/buildebpfPlugin.py
+++ b/tools/testing/selftests/tc-testing/plugin-lib/buildebpfPlugin.py
@@ -34,8 +34,9 @@ class SubPlugin(TdcPlugin):
             'buildebpf',
             'options for buildebpfPlugin')
         self.argparser_group.add_argument(
-            '-B', '--buildebpf', action='store_true',
-            help='build eBPF programs')
+            '--nobuildebpf', action='store_false', default=True,
+            dest='buildebpf',
+            help='Don\'t build eBPF programs')
 
         return self.argparser
 
diff --git a/tools/testing/selftests/tc-testing/plugin-lib/nsPlugin.py b/tools/testing/selftests/tc-testing/plugin-lib/nsPlugin.py
index a194b1a..affa7f2 100644
--- a/tools/testing/selftests/tc-testing/plugin-lib/nsPlugin.py
+++ b/tools/testing/selftests/tc-testing/plugin-lib/nsPlugin.py
@@ -18,6 +18,8 @@ class SubPlugin(TdcPlugin):
 
         if self.args.namespace:
             self._ns_create()
+        else:
+            self._ports_create()
 
     def post_suite(self, index):
         '''run commands after test_runner goes into a test loop'''
@@ -27,6 +29,8 @@ class SubPlugin(TdcPlugin):
 
         if self.args.namespace:
             self._ns_destroy()
+        else:
+            self._ports_destroy()
 
     def add_args(self, parser):
         super().add_args(parser)
@@ -34,8 +38,8 @@ class SubPlugin(TdcPlugin):
             'netns',
             'options for nsPlugin(run commands in net namespace)')
         self.argparser_group.add_argument(
-            '-n', '--namespace', action='store_true',
-            help='Run commands in namespace')
+            '-N', '--no-namespace', action='store_false', default=True,
+            dest='namespace', help='Don\'t run commands in namespace')
         return self.argparser
 
     def adjust_command(self, stage, command):
@@ -73,20 +77,30 @@ class SubPlugin(TdcPlugin):
             print('adjust_command:  return command [{}]'.format(command))
         return command
 
+    def _ports_create(self):
+        cmd = 'ip link add $DEV0 type veth peer name $DEV1'
+        self._exec_cmd('pre', cmd)
+        cmd = 'ip link set $DEV0 up'
+        self._exec_cmd('pre', cmd)
+        if not self.args.namespace:
+            cmd = 'ip link set $DEV1 up'
+            self._exec_cmd('pre', cmd)
+
+    def _ports_destroy(self):
+        cmd = 'ip link del $DEV0'
+        self._exec_cmd('post', cmd)
+
     def _ns_create(self):
         '''
         Create the network namespace in which the tests will be run and set up
         the required network devices for it.
         '''
+        self._ports_create()
         if self.args.namespace:
             cmd = 'ip netns add {}'.format(self.args.NAMES['NS'])
             self._exec_cmd('pre', cmd)
-            cmd = 'ip link add $DEV0 type veth peer name $DEV1'
-            self._exec_cmd('pre', cmd)
             cmd = 'ip link set $DEV1 netns {}'.format(self.args.NAMES['NS'])
             self._exec_cmd('pre', cmd)
-            cmd = 'ip link set $DEV0 up'
-            self._exec_cmd('pre', cmd)
             cmd = 'ip -n {} link set $DEV1 up'.format(self.args.NAMES['NS'])
             self._exec_cmd('pre', cmd)
             if self.args.device:
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json b/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json
index b074ea9..47a3082 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json
@@ -54,6 +54,9 @@
             "actions",
             "bpf"
         ],
+        "plugins": {
+                "requires": "buildebpfPlugin"
+        },
         "setup": [
             [
                 "$TC action flush action bpf",
@@ -78,6 +81,9 @@
             "actions",
             "bpf"
         ],
+        "plugins": {
+                "requires": "buildebpfPlugin"
+        },
         "setup": [
             [
                 "$TC action flush action bpf",
diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/fw.json b/tools/testing/selftests/tc-testing/tc-tests/filters/fw.json
index 6944b90..5272049 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/filters/fw.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/fw.json
@@ -6,6 +6,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress"
         ],
@@ -25,6 +28,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress"
         ],
@@ -44,6 +50,114 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress"
         ],
@@ -872,6 +986,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: protocol 802_3 prio 3 handle 7 fw action ok"
@@ -892,6 +1009,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: prio 6 handle 2 fw action continue index 5"
@@ -912,6 +1032,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress"
         ],
@@ -931,6 +1054,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress"
         ],
@@ -950,6 +1076,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 5 prio 7 fw action pass",
@@ -972,6 +1101,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 5 prio 7 fw action pass",
@@ -994,6 +1126,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 5 prio 7 fw action pass",
@@ -1015,6 +1150,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 1 prio 4 fw action ok",
@@ -1036,6 +1174,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 4 prio 2 chain 13 fw action pipe",
@@ -1057,6 +1198,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 2 prio 4 fw action drop"
@@ -1077,6 +1221,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 3 prio 4 fw action continue"
@@ -1097,6 +1244,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 4 prio 2 protocol arp fw action pipe"
@@ -1117,6 +1267,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 4 prio 2 fw action pipe flowid 45"
@@ -1137,6 +1290,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 1 prio 2 fw action ok"
@@ -1157,6 +1313,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 1 prio 2 fw action ok"
@@ -1177,6 +1336,9 @@
             "filter",
             "fw"
         ],
+	"plugins": {
+		"requires": "nsPlugin"
+	},
         "setup": [
             "$TC qdisc add dev $DEV1 ingress",
             "$TC filter add dev $DEV1 parent ffff: handle 1 prio 2 fw action ok index 3"
diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
index e2f92ce..8135778 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
@@ -6,6 +6,9 @@
             "filter",
             "u32"
         ],
+        "plugins": {
+                "requires": "nsPlugin"
+        },
         "setup": [
             "$TC qdisc add dev $DEV1 ingress"
         ],
@@ -25,6 +28,9 @@
             "filter",
             "matchall"
         ],
+        "plugins": {
+                "requires": "nsPlugin"
+        },
         "setup": [
             "$TC qdisc add dev $DEV1 clsact",
             "$TC filter add dev $DEV1 protocol all pref 1 ingress handle 0x1234 matchall action ok"
@@ -45,6 +51,9 @@
             "filter",
             "flower"
         ],
+        "plugins": {
+                "requires": "nsPlugin"
+        },
         "setup": [
             "$TC qdisc add dev $DEV2 ingress",
             "./tdc_batch.py $DEV2 $BATCH_FILE --share_action -n 1000000"
@@ -66,6 +75,9 @@
             "filter",
             "flower"
         ],
+        "plugins": {
+                "requires": "nsPlugin"
+        },
         "setup": [
             "$TC qdisc add dev $DEV2 ingress",
             "$TC filter add dev $DEV2 protocol ip prio 1 parent ffff: flower dst_mac e4:11:22:11:4a:51 src_mac e4:11:22:11:4a:50 ip_proto tcp src_ip 1.1.1.1 dst_ip 2.2.2.2 action drop"
diff --git a/tools/testing/selftests/tc-testing/tdc.py b/tools/testing/selftests/tc-testing/tdc.py
index 5cee156..678182a 100755
--- a/tools/testing/selftests/tc-testing/tdc.py
+++ b/tools/testing/selftests/tc-testing/tdc.py
@@ -25,6 +25,9 @@ from tdc_helper import *
 import TdcPlugin
 from TdcResults import *
 
+class PluginDependencyException(Exception):
+    def __init__(self, missing_pg):
+        self.missing_pg = missing_pg
 
 class PluginMgrTestFail(Exception):
     def __init__(self, stage, output, message):
@@ -37,7 +40,7 @@ class PluginMgr:
         super().__init__()
         self.plugins = {}
         self.plugin_instances = []
-        self.args = []
+        self.failed_plugins = {}
         self.argparser = argparser
 
         # TODO, put plugins in order
@@ -53,6 +56,64 @@ class PluginMgr:
                     self.plugins[mn] = foo
                     self.plugin_instances.append(foo.SubPlugin())
 
+    def load_plugin(self, pgdir, pgname):
+        pgname = pgname[0:-3]
+        foo = importlib.import_module('{}.{}'.format(pgdir, pgname))
+        self.plugins[pgname] = foo
+        self.plugin_instances.append(foo.SubPlugin())
+        self.plugin_instances[-1].check_args(self.args, None)
+
+    def get_required_plugins(self, testlist):
+        '''
+        Get all required plugins from the list of test cases and return
+        all unique items.
+        '''
+        reqs = []
+        for t in testlist:
+            try:
+                if 'requires' in t['plugins']:
+                    if isinstance(t['plugins']['requires'], list):
+                        reqs.extend(t['plugins']['requires'])
+                    else:
+                        reqs.append(t['plugins']['requires'])
+            except KeyError:
+                continue
+        reqs = get_unique_item(reqs)
+        return reqs
+
+    def load_required_plugins(self, reqs, parser, args, remaining):
+        '''
+        Get all required plugins from the list of test cases and load any plugin
+        that is not already enabled.
+        '''
+        pgd = ['plugin-lib', 'plugin-lib-custom']
+        pnf = []
+
+        for r in reqs:
+            if r not in self.plugins:
+                fname = '{}.py'.format(r)
+                source_path = []
+                for d in pgd:
+                    pgpath = '{}/{}'.format(d, fname)
+                    if os.path.isfile(pgpath):
+                        source_path.append(pgpath)
+                if len(source_path) == 0:
+                    print('ERROR: unable to find required plugin {}'.format(r))
+                    pnf.append(fname)
+                    continue
+                elif len(source_path) > 1:
+                    print('WARNING: multiple copies of plugin {} found, using version found')
+                    print('at {}'.format(source_path[0]))
+                pgdir = source_path[0]
+                pgdir = pgdir.split('/')[0]
+                self.load_plugin(pgdir, fname)
+        if len(pnf) > 0:
+            raise PluginDependencyException(pnf)
+
+        parser = self.call_add_args(parser)
+        (args, remaining) = parser.parse_known_args(args=remaining, namespace=args)
+        return args
+
     def call_pre_suite(self, testcount, testidlist):
         for pgn_inst in self.plugin_instances:
             pgn_inst.pre_suite(testcount, testidlist)
@@ -98,6 +159,9 @@ class PluginMgr:
             command = pgn_inst.adjust_command(stage, command)
         return command
 
+    def set_args(self, args):
+        self.args = args
+
     @staticmethod
     def _make_argparser(args):
         self.argparser = argparse.ArgumentParser(
@@ -550,6 +614,7 @@ def filter_tests_by_category(args, testlist):
 
     return answer
 
+
 def get_test_cases(args):
     """
     If a test case file is specified, retrieve tests from that file.
@@ -611,7 +676,7 @@ def get_test_cases(args):
     return allcatlist, allidlist, testcases_by_cats, alltestcases
 
 
-def set_operation_mode(pm, args):
+def set_operation_mode(pm, parser, args, remaining):
     """
     Load the test case data and process remaining arguments to determine
     what the script should do for this run, and call the appropriate
@@ -649,6 +714,12 @@ def set_operation_mode(pm, args):
             exit(0)
 
     if len(alltests):
+        req_plugins = pm.get_required_plugins(alltests)
+        try:
+            args = pm.load_required_plugins(req_plugins, parser, args, remaining)
+        except PluginDependencyException as pde:
+            print('The following plugins were not found:')
+            print('{}'.format(pde.missing_pg))
         catresults = test_runner(pm, args, alltests)
         if args.format == 'none':
             print('Test results output suppression requested\n')
@@ -686,11 +757,12 @@ def main():
     parser = pm.call_add_args(parser)
     (args, remaining) = parser.parse_known_args()
     args.NAMES = NAMES
+    pm.set_args(args)
     check_default_settings(args, remaining, pm)
     if args.verbose > 2:
         print('args is {}'.format(args))
 
-    set_operation_mode(pm, args)
+    set_operation_mode(pm, parser, args, remaining)
 
     exit(0)
 
diff --git a/tools/testing/selftests/tc-testing/tdc_helper.py b/tools/testing/selftests/tc-testing/tdc_helper.py
index 9f35c96..0440d25 100644
--- a/tools/testing/selftests/tc-testing/tdc_helper.py
+++ b/tools/testing/selftests/tc-testing/tdc_helper.py
@@ -17,7 +17,10 @@ def get_categorized_testlist(alltests, ucat):
 
 def get_unique_item(lst):
     """ For a list, return a list of the unique items in the list. """
-    return list(set(lst))
+    if len(lst) > 1:
+        return list(set(lst))
+    else:
+        return lst
 
 
 def get_test_categories(alltests):
-- 
2.7.4


^ permalink raw reply related

* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Daniel Santos @ 2019-06-25  0:58 UTC (permalink / raw)
  To: René van Dorst, sean.wang, f.fainelli, linux, davem,
	matthias.bgg, andrew, vivien.didelot
  Cc: frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190624145251.4849-2-opensource@vdorst.com>



On 6/24/19 9:52 AM, René van Dorst wrote:
> Convert mt7530 to PHYLINK API
>
> Signed-off-by: René van Dorst <opensource@vdorst.com>
> ---
>  drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
>  drivers/net/dsa/mt7530.h |   9 ++
>  2 files changed, 187 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3181e95586d6..9c5e4dd00826 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -13,7 +13,7 @@
>  #include <linux/of_mdio.h>
>  #include <linux/of_net.h>
>  #include <linux/of_platform.h>
> -#include <linux/phy.h>
> +#include <linux/phylink.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset)
>  	return ARRAY_SIZE(mt7530_mib);
>  }
>  
> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
> -			       struct phy_device *phydev)
> -{
> -	struct mt7530_priv *priv = ds->priv;
> -
> -	if (phy_is_pseudo_fixed_link(phydev)) {
> -		dev_dbg(priv->dev, "phy-mode for master device = %x\n",
> -			phydev->interface);
> -
> -		/* Setup TX circuit incluing relevant PAD and driving */
> -		mt7530_pad_clk_setup(ds, phydev->interface);
> -
> -		if (priv->id == ID_MT7530) {
> -			/* Setup RX circuit, relevant PAD and driving on the
> -			 * host which must be placed after the setup on the
> -			 * device side is all finished.
> -			 */
> -			mt7623_pad_clk_setup(ds);
> -		}
> -	} else {
> -		u16 lcl_adv = 0, rmt_adv = 0;
> -		u8 flowctrl;
> -		u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
> -
> -		switch (phydev->speed) {
> -		case SPEED_1000:
> -			mcr |= PMCR_FORCE_SPEED_1000;
> -			break;
> -		case SPEED_100:
> -			mcr |= PMCR_FORCE_SPEED_100;
> -			break;
> -		}
> -
> -		if (phydev->link)
> -			mcr |= PMCR_FORCE_LNK;
> -
> -		if (phydev->duplex) {
> -			mcr |= PMCR_FORCE_FDX;
> -
> -			if (phydev->pause)
> -				rmt_adv = LPA_PAUSE_CAP;
> -			if (phydev->asym_pause)
> -				rmt_adv |= LPA_PAUSE_ASYM;
> -
> -			lcl_adv = linkmode_adv_to_lcl_adv_t(
> -				phydev->advertising);
> -			flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
> -
> -			if (flowctrl & FLOW_CTRL_TX)
> -				mcr |= PMCR_TX_FC_EN;
> -			if (flowctrl & FLOW_CTRL_RX)
> -				mcr |= PMCR_RX_FC_EN;
> -		}
> -		mt7530_write(priv, MT7530_PMCR_P(port), mcr);
> -	}
> -}
> -
>  static int
>  mt7530_cpu_port_enable(struct mt7530_priv *priv,
>  		       int port)
> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
>  	return 0;
>  }
>  
> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
> +				      unsigned int mode,
> +				      const struct phylink_link_state *state)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
> +		  PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
> +
> +	switch (port) {
> +	case 0: /* Internal phy */
> +	case 1:
> +	case 2:
> +	case 3:
> +	case 4:
> +		if (state->interface != PHY_INTERFACE_MODE_GMII)
> +			goto unsupported;
> +		break;
> +	/* case 5: Port 5 is not supported! */
> +	case 6: /* 1st cpu port */
> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +		    state->interface != PHY_INTERFACE_MODE_TRGMII)
> +			goto unsupported;
> +
> +		/* Setup TX circuit incluing relevant PAD and driving */
> +		mt7530_pad_clk_setup(ds, state->interface);
> +
> +		if (priv->id == ID_MT7530) {
> +			/* Setup RX circuit, relevant PAD and driving on the
> +			 * host which must be placed after the setup on the
> +			 * device side is all finished.
> +			 */
> +			mt7623_pad_clk_setup(ds);
> +		}
> +		break;
> +	default:
> +		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> +		return;
> +	}
> +
> +	if (!state->an_enabled || mode == MLO_AN_FIXED) {
> +		mcr |= PMCR_FORCE_MODE;
> +
> +		if (state->speed == SPEED_1000)
> +			mcr |= PMCR_FORCE_SPEED_1000;
> +		if (state->speed == SPEED_100)
> +			mcr |= PMCR_FORCE_SPEED_100;

I would suggest using the defines

#define PMCR_FORCE_SPEED	0x0000000c /* or PMCR_FORCE_SPEED_MASK */
#define PMCR_FORCE_SPEED_10	0x00000000
#define PMCR_FORCE_SPEED_100	0x00000004
#define PMCR_FORCE_SPEED_1000	0x00000008

and a switch statement such as

	switch (state->speed) {
	case SPEED_10:
		mcr |= PMCR_FORCE_SPEED_10;
		break;
	case SPEED_100:
		mcr |= PMCR_FORCE_SPEED_100;
		break;
	case SPEED_1000:
		mcr |= PMCR_FORCE_SPEED_1000;
		break;
	}

This will compile the same (i.e, the mcr |= 0 will optimize away, etc.),
while alleviating the need to intimately know the hardware in order to
easily understand what the code is doing at a glance.  It's also better
form as we're treating the two bits as a composite value, rather than
two separate bits.


> +		if (state->duplex == DUPLEX_FULL)
> +			mcr |= PMCR_FORCE_FDX;
> +		if (state->link || mode == MLO_AN_FIXED)
> +			mcr |= PMCR_FORCE_LNK;
> +		if (state->pause || phylink_test(state->advertising, Pause))
> +			mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
> +		if (state->pause & MLO_PAUSE_TX)
> +			mcr |= PMCR_TX_FC_EN;
> +		if (state->pause & MLO_PAUSE_RX)
> +			mcr |= PMCR_RX_FC_EN;
> +	}
> +
> +	mt7530_write(priv, MT7530_PMCR_P(port), mcr);
> +
> +	return;
> +
> +unsupported:
> +	dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
> +		__func__, port, state->interface, phy_modes(state->interface));
> +}
> +
> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
> +					 unsigned int mode,
> +					 phy_interface_t interface)
> +{
> +	/* Do nothing */
> +}
> +
> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
> +				       unsigned int mode,
> +				       phy_interface_t interface,
> +				       struct phy_device *phydev)
> +{
> +	/* Do nothing */
> +}
> +
> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
> +				    unsigned long *supported,
> +				    struct phylink_link_state *state)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +	switch (port) {
> +	case 0: /* Internal phy */
> +	case 1:
> +	case 2:
> +	case 3:
> +	case 4:
> +		if (state->interface != PHY_INTERFACE_MODE_NA &&
> +		    state->interface != PHY_INTERFACE_MODE_GMII)
> +			goto unsupported;
> +		break;
> +	/* case 5: Port 5 not supported! */
> +	case 6: /* 1st cpu port */
> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +		    state->interface != PHY_INTERFACE_MODE_TRGMII)
> +			goto unsupported;
> +		break;
> +	default:
> +		linkmode_zero(supported);
> +		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> +		return;
> +	}
> +
> +	phylink_set(mask, Autoneg);
> +	phylink_set(mask, Pause);
> +	phylink_set(mask, Asym_Pause);
> +	phylink_set(mask, MII);
> +
> +	phylink_set(mask, 10baseT_Half);
> +	phylink_set(mask, 10baseT_Full);
> +	phylink_set(mask, 100baseT_Half);
> +	phylink_set(mask, 100baseT_Full);
> +	phylink_set(mask, 1000baseT_Full);
> +	phylink_set(mask, 1000baseT_Half);
> +
> +	linkmode_and(supported, supported, mask);
> +	linkmode_and(state->advertising, state->advertising, mask);
> +	return;
> +
> +unsupported:
> +	linkmode_zero(supported);
> +	dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
> +		__func__, state->interface, phy_modes(state->interface));
> +}
> +
> +static int
> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
> +			      struct phylink_link_state *state)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	u32 pmsr;
> +
> +	if (port < 0 || port >= MT7530_NUM_PORTS)
> +		return -EINVAL;
> +
> +	pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
> +
> +	state->link = (pmsr & PMSR_LINK);
> +	state->an_complete = state->link;
> +	state->duplex = (pmsr & PMSR_DPX) >> 1;
> +
> +	switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
> +	case 0:
> +		state->speed = SPEED_10;
> +		break;
> +	case PMSR_SPEED_100:
> +		state->speed = SPEED_100;
> +		break;
> +	case PMSR_SPEED_1000:
> +		state->speed = SPEED_1000;
> +		break;
> +	default:
> +		state->speed = SPEED_UNKNOWN;
> +		break;
> +	}
> +

Same as above: add PMSR_SPEED_10, and and with PMSR_SPEED (or
PMSR_SPEED_MASK if you prefer).

> +	state->pause = 0;
> +	if (pmsr & PMSR_RX_FC)
> +		state->pause |= MLO_PAUSE_RX;
> +	if (pmsr & PMSR_TX_FC)
> +		state->pause |= MLO_PAUSE_TX;
> +
> +	return 1;
> +}
> +
>  static const struct dsa_switch_ops mt7530_switch_ops = {
>  	.get_tag_protocol	= mtk_get_tag_protocol,
>  	.setup			= mt7530_setup,
> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>  	.phy_write		= mt7530_phy_write,
>  	.get_ethtool_stats	= mt7530_get_ethtool_stats,
>  	.get_sset_count		= mt7530_get_sset_count,
> -	.adjust_link		= mt7530_adjust_link,
>  	.port_enable		= mt7530_port_enable,
>  	.port_disable		= mt7530_port_disable,
>  	.port_stp_state_set	= mt7530_stp_state_set,
> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>  	.port_vlan_prepare	= mt7530_port_vlan_prepare,
>  	.port_vlan_add		= mt7530_port_vlan_add,
>  	.port_vlan_del		= mt7530_port_vlan_del,
> +	.phylink_validate	= mt7530_phylink_validate,
> +	.phylink_mac_link_state = mt7530_phylink_mac_link_state,
> +	.phylink_mac_config	= mt7530_phylink_mac_config,
> +	.phylink_mac_link_down	= mt7530_phylink_mac_link_down,
> +	.phylink_mac_link_up	= mt7530_phylink_mac_link_up,
>  };
>  
>  static const struct of_device_id mt7530_of_match[] = {
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index bfac90f48102..41d9a132ac70 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
>  #define  PMCR_FORCE_SPEED_100		BIT(2)
>  #define  PMCR_FORCE_FDX			BIT(1)
>  #define  PMCR_FORCE_LNK			BIT(0)
> +#define  PMCR_FORCE_LNK_DOWN		PMCR_FORCE_MODE
>  #define  PMCR_COMMON_LINK		(PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>  					 PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>  					 PMCR_TX_EN | PMCR_RX_EN | \
> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
>  					 PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>  
>  #define MT7530_PMSR_P(x)		(0x3008 + (x) * 0x100)
> +#define  PMSR_EEE1G			BIT(7)
> +#define  PMSR_EEE100M			BIT(6)
> +#define  PMSR_RX_FC			BIT(5)
> +#define  PMSR_TX_FC			BIT(4)
> +#define  PMSR_SPEED_1000		BIT(3)
> +#define  PMSR_SPEED_100			BIT(2)
> +#define  PMSR_DPX			BIT(1)
> +#define  PMSR_LINK			BIT(0)
>  
>  /* Register for MIB */
>  #define MT7530_PORT_MIB_COUNTER(x)	(0x4000 + (x) * 0x100)

Cheers,
Daniel

^ permalink raw reply

* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Jakub Kicinski @ 2019-06-25  0:57 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
	netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
	Stanislav Fomichev
In-Reply-To: <20190624174726.2dda122b@cakuba.netronome.com>

On Mon, 24 Jun 2019 17:47:26 -0700, Jakub Kicinski wrote:
> I see.  The local flag would not an option in getopt_long() sense, what
> I was thinking was about adding an "effective" keyword:

Something like this, untested:

--->8------------

The BPF_F_QUERY_EFFECTIVE is a syscall flag, and fits nicely
as a subcommand option.  We want to move away from global
options, anyway.

We need a global variable because of nftw limitations.
Clean this flag on every invocations in case we run in
batch mode.

NOTE the argv[1] use on the error path in do_show() looks
like a bug on it's own.
---
 .../bpftool/Documentation/bpftool-cgroup.rst  | 24 +++----
 tools/bpf/bpftool/Documentation/bpftool.rst   |  6 +-
 tools/bpf/bpftool/bash-completion/bpftool     | 17 ++---
 tools/bpf/bpftool/cgroup.c                    | 62 ++++++++++++-------
 tools/bpf/bpftool/main.c                      |  7 +--
 tools/bpf/bpftool/main.h                      |  3 +-
 6 files changed, 66 insertions(+), 53 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
index 324df15bf4cc..4fde3dfad395 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
@@ -12,8 +12,7 @@ SYNOPSIS
 
 	**bpftool** [*OPTIONS*] **cgroup** *COMMAND*
 
-	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** }
-	| { **-e** | **--effective** } }
+	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
 
 	*COMMANDS* :=
 	{ **show** | **list** | **tree** | **attach** | **detach** | **help** }
@@ -21,8 +20,8 @@ SYNOPSIS
 CGROUP COMMANDS
 ===============
 
-|	**bpftool** **cgroup { show | list }** *CGROUP*
-|	**bpftool** **cgroup tree** [*CGROUP_ROOT*]
+|	**bpftool** **cgroup { show | list }** *CGROUP* [**effective**]
+|	**bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**]
 |	**bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
 |	**bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
 |	**bpftool** **cgroup help**
@@ -35,13 +34,17 @@ CGROUP COMMANDS
 
 DESCRIPTION
 ===========
-	**bpftool cgroup { show | list }** *CGROUP*
+	**bpftool cgroup { show | list }** *CGROUP* [**effective**]
 		  List all programs attached to the cgroup *CGROUP*.
 
 		  Output will start with program ID followed by attach type,
 		  attach flags and program name.
 
-	**bpftool cgroup tree** [*CGROUP_ROOT*]
+		  If **effective** is specified retrieve effective programs that
+		  will execute for events within a cgroup. This includes
+		  inherited along with attached ones.
+
+	**bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**]
 		  Iterate over all cgroups in *CGROUP_ROOT* and list all
 		  attached programs. If *CGROUP_ROOT* is not specified,
 		  bpftool uses cgroup v2 mountpoint.
@@ -50,6 +53,10 @@ DESCRIPTION
 		  commands: it starts with absolute cgroup path, followed by
 		  program ID, attach type, attach flags and program name.
 
+		  If **effective** is specified retrieve effective programs that
+		  will execute for events within a cgroup. This includes
+		  inherited along with attached ones.
+
 	**bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
 		  Attach program *PROG* to the cgroup *CGROUP* with attach type
 		  *ATTACH_TYPE* and optional *ATTACH_FLAGS*.
@@ -122,11 +129,6 @@ OPTIONS
 		  Print all logs available from libbpf, including debug-level
 		  information.
 
-	-e, --effective
-		  Retrieve effective programs that will execute for events
-		  within a cgroup. This includes inherited along with attached
-		  ones.
-
 EXAMPLES
 ========
 |
diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
index d2f76b55988d..6a9c52ef84a9 100644
--- a/tools/bpf/bpftool/Documentation/bpftool.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool.rst
@@ -19,7 +19,7 @@ SYNOPSIS
 	*OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** | **feature** }
 
 	*OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** }
-	| { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** | **--effective** } }
+	| { **-j** | **--json** } [{ **-p** | **--pretty** }] }
 
 	*MAP-COMMANDS* :=
 	{ **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext**
@@ -71,10 +71,6 @@ OPTIONS
 		  includes logs from libbpf as well as from the verifier, when
 		  attempting to load programs.
 
-	-e, --effective
-		  Retrieve effective programs that will execute for events
-		  within a cgroup. This includes inherited along with attached ones.
-
 SEE ALSO
 ========
 	**bpf**\ (2),
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index c98cb99867f6..de84ae06ae4e 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -187,7 +187,7 @@ _bpftool()
 
     # Deal with options
     if [[ ${words[cword]} == -* ]]; then
-        local c='--version --json --pretty --bpffs --mapcompat --debug --effective'
+        local c='--version --json --pretty --bpffs --mapcompat --debug'
         COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
         return 0
     fi
@@ -678,12 +678,15 @@ _bpftool()
             ;;
         cgroup)
             case $command in
-                show|list)
-                    _filedir
-                    return 0
-                    ;;
-                tree)
-                    _filedir
+                show|list|tree)
+                    case $cword in
+                        3)
+                            _filedir
+                            ;;
+                        4)
+                            COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) )
+                            ;;
+                    esac
                     return 0
                     ;;
                 attach|detach)
diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
index 1bb2a751107a..88b80616d47b 100644
--- a/tools/bpf/bpftool/cgroup.c
+++ b/tools/bpf/bpftool/cgroup.c
@@ -28,6 +28,8 @@
 	"                        connect6 | sendmsg4 | sendmsg6 |\n"           \
 	"                        recvmsg4 | recvmsg6 | sysctl }"
 
+static unsigned int query_flags;
+
 static const char * const attach_type_strings[] = {
 	[BPF_CGROUP_INET_INGRESS] = "ingress",
 	[BPF_CGROUP_INET_EGRESS] = "egress",
@@ -104,8 +106,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
 	__u32 prog_cnt = 0;
 	int ret;
 
-	ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL,
-			     &prog_cnt);
+	ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL,
+			     NULL, &prog_cnt);
 	if (ret)
 		return -1;
 
@@ -156,20 +158,30 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
 static int do_show(int argc, char **argv)
 {
 	enum bpf_attach_type type;
+	const char *path;
 	int cgroup_fd;
 	int ret = -1;
 
-	if (argc < 1) {
-		p_err("too few parameters for cgroup show");
-		goto exit;
-	} else if (argc > 1) {
-		p_err("too many parameters for cgroup show");
-		goto exit;
+	query_flags = 0;
+
+	if (!REQ_ARGS(1))
+		return -1;
+	path = GET_ARG();
+
+	while (argc) {
+		if (is_prefix(*argv, "effective")) {
+			query_flags |= BPF_F_QUERY_EFFECTIVE;
+			NEXT_ARG();
+		} else {
+			p_err("expected no more arguments, 'effective', got: '%s'?",
+			      *argv);
+			return -1;
+		}
 	}
 
-	cgroup_fd = open(argv[0], O_RDONLY);
+	cgroup_fd = open(path, O_RDONLY);
 	if (cgroup_fd < 0) {
-		p_err("can't open cgroup %s", argv[1]);
+		p_err("can't open cgroup %s", path);
 		goto exit;
 	}
 
@@ -295,23 +307,29 @@ static int do_show_tree(int argc, char **argv)
 	char *cgroup_root;
 	int ret;
 
-	switch (argc) {
-	case 0:
+	query_flags = 0;
+
+	if (!argc) {
 		cgroup_root = find_cgroup_root();
 		if (!cgroup_root) {
 			p_err("cgroup v2 isn't mounted");
 			return -1;
 		}
-		break;
-	case 1:
-		cgroup_root = argv[0];
-		break;
-	default:
-		p_err("too many parameters for cgroup tree");
-		return -1;
+	} else {
+		cgroup_root = GET_ARG();
+
+		while (argc) {
+			if (is_prefix(*argv, "effective")) {
+				query_flags |= BPF_F_QUERY_EFFECTIVE;
+				NEXT_ARG();
+			} else {
+				p_err("expected no more arguments, 'effective', got: '%s'?",
+				      *argv);
+				return -1;
+			}
+		}
 	}
 
-
 	if (json_output)
 		jsonw_start_array(json_wtr);
 	else
@@ -457,8 +475,8 @@ static int do_help(int argc, char **argv)
 	}
 
 	fprintf(stderr,
-		"Usage: %s %s { show | list } CGROUP\n"
-		"       %s %s tree [CGROUP_ROOT]\n"
+		"Usage: %s %s { show | list } CGROUP [**effective**]\n"
+		"       %s %s tree [CGROUP_ROOT] [**effective**]\n"
 		"       %s %s attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]\n"
 		"       %s %s detach CGROUP ATTACH_TYPE PROG\n"
 		"       %s %s help\n"
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 42e9ddfbbbe0..4879f6395c7e 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -27,7 +27,6 @@ bool json_output;
 bool show_pinned;
 bool block_mount;
 bool verifier_logs;
-unsigned int query_flags;
 int bpf_flags;
 struct pinned_obj_table prog_table;
 struct pinned_obj_table map_table;
@@ -328,7 +327,6 @@ int main(int argc, char **argv)
 		{ "mapcompat",	no_argument,	NULL,	'm' },
 		{ "nomount",	no_argument,	NULL,	'n' },
 		{ "debug",	no_argument,	NULL,	'd' },
-		{ "effective",	no_argument,	NULL,	'e' },
 		{ 0 }
 	};
 	int opt, ret;
@@ -344,7 +342,7 @@ int main(int argc, char **argv)
 	hash_init(map_table.table);
 
 	opterr = 0;
-	while ((opt = getopt_long(argc, argv, "Vhpjfmnde",
+	while ((opt = getopt_long(argc, argv, "Vhpjfmnd",
 				  options, NULL)) >= 0) {
 		switch (opt) {
 		case 'V':
@@ -378,9 +376,6 @@ int main(int argc, char **argv)
 			libbpf_set_print(print_all_levels);
 			verifier_logs = true;
 			break;
-		case 'e':
-			query_flags = BPF_F_QUERY_EFFECTIVE;
-			break;
 		default:
 			p_err("unrecognized option '%s'", argv[optind - 1]);
 			if (json_output)
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index fddec15c454a..28a2a5857e14 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -45,7 +45,7 @@
 	"PROG := { id PROG_ID | pinned FILE | tag PROG_TAG }"
 #define HELP_SPEC_OPTIONS						\
 	"OPTIONS := { {-j|--json} [{-p|--pretty}] | {-f|--bpffs} |\n"	\
-	"\t            {-m|--mapcompat} | {-n|--nomount} | {-e|--effective} }"
+	"\t            {-m|--mapcompat} | {-n|--nomount} }"
 #define HELP_SPEC_MAP							\
 	"MAP := { id MAP_ID | pinned FILE }"
 
@@ -92,7 +92,6 @@ extern bool json_output;
 extern bool show_pinned;
 extern bool block_mount;
 extern bool verifier_logs;
-extern unsigned int query_flags;
 extern int bpf_flags;
 extern struct pinned_obj_table prog_table;
 extern struct pinned_obj_table map_table;
-- 
2.21.0


^ permalink raw reply related

* [PATCH v2] samples: bpf: make the use of xdp samples consistent
From: Daniel T. Lee @ 2019-06-25  0:55 UTC (permalink / raw)
  To: Daniel Borkmann, Alexei Starovoitov; +Cc: bpf, netdev

Currently, each xdp samples are inconsistent in the use.
Most of the samples fetch the interface with it's name.
(ex. xdp1, xdp2skb, xdp_redirect_cpu, xdp_sample_pkts, etc.)

But some of the xdp samples are fetching the interface with
ifindex by command argument.

This commit enables xdp samples to fetch interface with it's name
without changing the original index interface fetching.
(<ifname|ifindex> fetching in the same way as xdp_sample_pkts_user.c does.)

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
Changes in v2:
  - added xdp_redirect_user.c, xdp_redirect_map_user.c 

 samples/bpf/xdp_adjust_tail_user.c  | 12 ++++++++++--
 samples/bpf/xdp_redirect_map_user.c | 15 +++++++++++----
 samples/bpf/xdp_redirect_user.c     | 15 +++++++++++----
 samples/bpf/xdp_tx_iptunnel_user.c  | 12 ++++++++++--
 4 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/samples/bpf/xdp_adjust_tail_user.c b/samples/bpf/xdp_adjust_tail_user.c
index 586ff751aba9..a3596b617c4c 100644
--- a/samples/bpf/xdp_adjust_tail_user.c
+++ b/samples/bpf/xdp_adjust_tail_user.c
@@ -13,6 +13,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <net/if.h>
 #include <sys/resource.h>
 #include <arpa/inet.h>
 #include <netinet/ether.h>
@@ -69,7 +70,7 @@ static void usage(const char *cmd)
 	printf("Start a XDP prog which send ICMP \"packet too big\" \n"
 		"messages if ingress packet is bigger then MAX_SIZE bytes\n");
 	printf("Usage: %s [...]\n", cmd);
-	printf("    -i <ifindex> Interface Index\n");
+	printf("    -i <ifname|ifindex> Interface\n");
 	printf("    -T <stop-after-X-seconds> Default: 0 (forever)\n");
 	printf("    -S use skb-mode\n");
 	printf("    -N enforce native mode\n");
@@ -102,7 +103,9 @@ int main(int argc, char **argv)
 
 		switch (opt) {
 		case 'i':
-			ifindex = atoi(optarg);
+			ifindex = if_nametoindex(optarg);
+			if (!ifindex)
+				ifindex = atoi(optarg);
 			break;
 		case 'T':
 			kill_after_s = atoi(optarg);
@@ -136,6 +139,11 @@ int main(int argc, char **argv)
 		return 1;
 	}
 
+	if (!ifindex) {
+		fprintf(stderr, "Invalid ifname\n");
+		return 1;
+	}
+
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 	prog_load_attr.file = filename;
 
diff --git a/samples/bpf/xdp_redirect_map_user.c b/samples/bpf/xdp_redirect_map_user.c
index 15bb6f67f9c3..f70ee33907fd 100644
--- a/samples/bpf/xdp_redirect_map_user.c
+++ b/samples/bpf/xdp_redirect_map_user.c
@@ -10,6 +10,7 @@
 #include <stdlib.h>
 #include <stdbool.h>
 #include <string.h>
+#include <net/if.h>
 #include <unistd.h>
 #include <libgen.h>
 #include <sys/resource.h>
@@ -85,7 +86,7 @@ static void poll_stats(int interval, int ifindex)
 static void usage(const char *prog)
 {
 	fprintf(stderr,
-		"usage: %s [OPTS] IFINDEX_IN IFINDEX_OUT\n\n"
+		"usage: %s [OPTS] <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n\n"
 		"OPTS:\n"
 		"    -S    use skb-mode\n"
 		"    -N    enforce native mode\n"
@@ -127,7 +128,7 @@ int main(int argc, char **argv)
 	}
 
 	if (optind == argc) {
-		printf("usage: %s IFINDEX_IN IFINDEX_OUT\n", argv[0]);
+		printf("usage: %s <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n", argv[0]);
 		return 1;
 	}
 
@@ -136,8 +137,14 @@ int main(int argc, char **argv)
 		return 1;
 	}
 
-	ifindex_in = strtoul(argv[optind], NULL, 0);
-	ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+	ifindex_in = if_nametoindex(argv[optind]);
+	if (!ifindex_in)
+		ifindex_in = strtoul(argv[optind], NULL, 0);
+
+	ifindex_out = if_nametoindex(argv[optind + 1]);
+	if (!ifindex_out)
+		ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+
 	printf("input: %d output: %d\n", ifindex_in, ifindex_out);
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
diff --git a/samples/bpf/xdp_redirect_user.c b/samples/bpf/xdp_redirect_user.c
index ce71be187205..39de06f3ec25 100644
--- a/samples/bpf/xdp_redirect_user.c
+++ b/samples/bpf/xdp_redirect_user.c
@@ -10,6 +10,7 @@
 #include <stdlib.h>
 #include <stdbool.h>
 #include <string.h>
+#include <net/if.h>
 #include <unistd.h>
 #include <libgen.h>
 #include <sys/resource.h>
@@ -85,7 +86,7 @@ static void poll_stats(int interval, int ifindex)
 static void usage(const char *prog)
 {
 	fprintf(stderr,
-		"usage: %s [OPTS] IFINDEX_IN IFINDEX_OUT\n\n"
+		"usage: %s [OPTS] <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n\n"
 		"OPTS:\n"
 		"    -S    use skb-mode\n"
 		"    -N    enforce native mode\n"
@@ -128,7 +129,7 @@ int main(int argc, char **argv)
 	}
 
 	if (optind == argc) {
-		printf("usage: %s IFINDEX_IN IFINDEX_OUT\n", argv[0]);
+		printf("usage: %s <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n", argv[0]);
 		return 1;
 	}
 
@@ -137,8 +138,14 @@ int main(int argc, char **argv)
 		return 1;
 	}
 
-	ifindex_in = strtoul(argv[optind], NULL, 0);
-	ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+	ifindex_in = if_nametoindex(argv[optind]);
+	if (!ifindex_in)
+		ifindex_in = strtoul(argv[optind], NULL, 0);
+
+	ifindex_out = if_nametoindex(argv[optind + 1]);
+	if (!ifindex_out)
+		ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+
 	printf("input: %d output: %d\n", ifindex_in, ifindex_out);
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
diff --git a/samples/bpf/xdp_tx_iptunnel_user.c b/samples/bpf/xdp_tx_iptunnel_user.c
index 394896430712..dfb68582e243 100644
--- a/samples/bpf/xdp_tx_iptunnel_user.c
+++ b/samples/bpf/xdp_tx_iptunnel_user.c
@@ -9,6 +9,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <net/if.h>
 #include <sys/resource.h>
 #include <arpa/inet.h>
 #include <netinet/ether.h>
@@ -83,7 +84,7 @@ static void usage(const char *cmd)
 	       "in an IPv4/v6 header and XDP_TX it out.  The dst <VIP:PORT>\n"
 	       "is used to select packets to encapsulate\n\n");
 	printf("Usage: %s [...]\n", cmd);
-	printf("    -i <ifindex> Interface Index\n");
+	printf("    -i <ifname|ifindex> Interface\n");
 	printf("    -a <vip-service-address> IPv4 or IPv6\n");
 	printf("    -p <vip-service-port> A port range (e.g. 433-444) is also allowed\n");
 	printf("    -s <source-ip> Used in the IPTunnel header\n");
@@ -181,7 +182,9 @@ int main(int argc, char **argv)
 
 		switch (opt) {
 		case 'i':
-			ifindex = atoi(optarg);
+			ifindex = if_nametoindex(optarg);
+			if (!ifindex)
+				ifindex = atoi(optarg);
 			break;
 		case 'a':
 			vip.family = parse_ipstr(optarg, vip.daddr.v6);
@@ -253,6 +256,11 @@ int main(int argc, char **argv)
 		return 1;
 	}
 
+	if (!ifindex) {
+		fprintf(stderr, "Invalid ifname\n");
+		return 1;
+	}
+
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 	prog_load_attr.file = filename;
 
-- 
2.17.1


^ permalink raw reply related

* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Jakub Kicinski @ 2019-06-25  0:47 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
	netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
	Stanislav Fomichev
In-Reply-To: <01c2c76b-5a45-aab0-e698-b5a66ab6c2e7@fb.com>

On Tue, 25 Jun 2019 00:40:09 +0000, Alexei Starovoitov wrote:
> On 6/24/19 5:30 PM, Jakub Kicinski wrote:
> > On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote:  
> >> On 6/24/19 5:16 PM, Jakub Kicinski wrote:  
> >>> On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote:  
> >>>> I don't think this patch should be penalized.
> >>>> I'd rather see we fix them all.  
> >>>
> >>> So we are going to add this broken option just to remove it?
> >>> I don't understand.
> >>> I'm happy to spend the 15 minutes rewriting this if you don't
> >>> want to penalize Takshak.  
> >>
> >> hmm. I don't understand the 'broken' part.
> >> The only issue I see that it could have been local vs global,
> >> but they all should have been local.  
> > 
> > I don't think all of them.  Only --mapcompat and --bpffs.  bpffs could
> > be argued.  On mapcompat I must have not read the patch fully, I was
> > under the impression its a global libbpf flag :(
> > 
> > --json, --pretty, --nomount, --debug are global because they affect
> > global behaviour of bpftool.  The difference here is that we basically
> > add a syscall parameter as a global option.  
> 
> sure. I only disagreed about not touching older flags.
> --effective should be local.
> If follow up patch means 90% rewrite then revert is better.
> If it's 10% fixup then it's different story.

I see.  The local flag would not an option in getopt_long() sense, what
I was thinking was about adding an "effective" keyword:

# bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/

becomes:

# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ effective

That's how we handle flags for update calls for instance..

So I think a revert :(

> Takshak,
> could you check which way is cleaner? Revert and new patch or follow up fix?
> But bpftool doesn't have a way to do local, no?
> so it's kinda new feature and other flags should become local too.
> hence it feels more like follow up. Just my .02


^ permalink raw reply

* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Alexei Starovoitov @ 2019-06-25  0:40 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
	netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
	Stanislav Fomichev
In-Reply-To: <20190624173005.06430163@cakuba.netronome.com>

On 6/24/19 5:30 PM, Jakub Kicinski wrote:
> On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote:
>> On 6/24/19 5:16 PM, Jakub Kicinski wrote:
>>> On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote:
>>>> I don't think this patch should be penalized.
>>>> I'd rather see we fix them all.
>>>
>>> So we are going to add this broken option just to remove it?
>>> I don't understand.
>>> I'm happy to spend the 15 minutes rewriting this if you don't
>>> want to penalize Takshak.
>>
>> hmm. I don't understand the 'broken' part.
>> The only issue I see that it could have been local vs global,
>> but they all should have been local.
> 
> I don't think all of them.  Only --mapcompat and --bpffs.  bpffs could
> be argued.  On mapcompat I must have not read the patch fully, I was
> under the impression its a global libbpf flag :(
> 
> --json, --pretty, --nomount, --debug are global because they affect
> global behaviour of bpftool.  The difference here is that we basically
> add a syscall parameter as a global option.

sure. I only disagreed about not touching older flags.
--effective should be local.
If follow up patch means 90% rewrite then revert is better.
If it's 10% fixup then it's different story.

Takshak,
could you check which way is cleaner? Revert and new patch or follow up fix?
But bpftool doesn't have a way to do local, no?
so it's kinda new feature and other flags should become local too.
hence it feels more like follow up. Just my .02

^ permalink raw reply

* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Jakub Kicinski @ 2019-06-25  0:30 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
	netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
	Stanislav Fomichev
In-Reply-To: <6d44d265-7133-d191-beeb-c22dde73993f@fb.com>

On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote:
> On 6/24/19 5:16 PM, Jakub Kicinski wrote:
> > On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote:  
> >> I don't think this patch should be penalized.
> >> I'd rather see we fix them all.  
> > 
> > So we are going to add this broken option just to remove it?
> > I don't understand.
> > I'm happy to spend the 15 minutes rewriting this if you don't
> > want to penalize Takshak.  
> 
> hmm. I don't understand the 'broken' part.
> The only issue I see that it could have been local vs global,
> but they all should have been local.

I don't think all of them.  Only --mapcompat and --bpffs.  bpffs could
be argued.  On mapcompat I must have not read the patch fully, I was
under the impression its a global libbpf flag :(

--json, --pretty, --nomount, --debug are global because they affect
global behaviour of bpftool.  The difference here is that we basically
add a syscall parameter as a global option.

^ permalink raw reply

* Re: [PATCH RFC net-next 5/5] net: dsa: mt7530: Add mediatek,ephy-handle to isolate external phy
From: Daniel Santos @ 2019-06-25  0:22 UTC (permalink / raw)
  To: Andrew Lunn, René van Dorst
  Cc: sean.wang, f.fainelli, linux, davem, matthias.bgg, vivien.didelot,
	frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190624215248.GC31306@lunn.ch>

On 6/24/19 4:52 PM, Andrew Lunn wrote:
>> +static int mt7530_isolate_ephy(struct dsa_switch *ds,
>> +			       struct device_node *ephy_node)
>> +{
>> +	struct phy_device *phydev = of_phy_find_device(ephy_node);
>> +	int ret;
>> +
>> +	if (!phydev)
>> +		return 0;
>> +
>> +	ret = phy_modify(phydev, MII_BMCR, 0, (BMCR_ISOLATE | BMCR_PDOWN));
> genphy_suspend() does what you want.
>
>> +	if (ret)
>> +		dev_err(ds->dev, "Failed to put phy %s in isolation mode!\n",
>> +			ephy_node->full_name);
>> +	else
>> +		dev_info(ds->dev, "Phy %s in isolation mode!\n",
>> +			 ephy_node->full_name);
> No need to clog up the system with yet more kernel messages.
>
>    Andrew
>
Yes, keep in mind that many mt7530-based devices have a 56k serial
console that gets ring buffer spew.  This created a real problem on the
mt7620 wifi drivers when they spewed every time a packet needed to be
dropped.  So at the very least, for any message that can be spammed,
rate limit it please.

Daniel

^ permalink raw reply

* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Alexei Starovoitov @ 2019-06-25  0:21 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
	netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
	Stanislav Fomichev
In-Reply-To: <20190624171641.73cd197d@cakuba.netronome.com>

On 6/24/19 5:16 PM, Jakub Kicinski wrote:
> On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote:
>> I don't think this patch should be penalized.
>> I'd rather see we fix them all.
> 
> So we are going to add this broken option just to remove it?
> I don't understand.
> I'm happy to spend the 15 minutes rewriting this if you don't
> want to penalize Takshak.

hmm. I don't understand the 'broken' part.
The only issue I see that it could have been local vs global,
but they all should have been local.



^ permalink raw reply

* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Jakub Kicinski @ 2019-06-25  0:16 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
	netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
	Stanislav Fomichev
In-Reply-To: <97b13eb6-43fb-8ee9-117d-a68f9825b866@fb.com>

On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote:
> I don't think this patch should be penalized.
> I'd rather see we fix them all.

So we are going to add this broken option just to remove it?
I don't understand.  
I'm happy to spend the 15 minutes rewriting this if you don't 
want to penalize Takshak.

^ permalink raw reply

* [PATCH 03/26] netfilter: ipset: merge uadd and udel functions
From: Pablo Neira Ayuso @ 2019-06-25  0:12 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <20190625001233.22057-1-pablo@netfilter.org>

From: Florent Fourcot <florent.fourcot@wifirst.fr>

Both functions are using exactly the same code, except the command value
passed to call_ad function.

Signed-off-by: Florent Fourcot <florent.fourcot@wifirst.fr>
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 net/netfilter/ipset/ip_set_core.c | 71 +++++++++++----------------------------
 1 file changed, 20 insertions(+), 51 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index faddcf398b73..2ad609900b22 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1561,10 +1561,12 @@ call_ad(struct sock *ctnl, struct sk_buff *skb, struct ip_set *set,
 	return ret;
 }
 
-static int ip_set_uadd(struct net *net, struct sock *ctnl, struct sk_buff *skb,
-		       const struct nlmsghdr *nlh,
-		       const struct nlattr * const attr[],
-		       struct netlink_ext_ack *extack)
+static int ip_set_ad(struct net *net, struct sock *ctnl,
+		     struct sk_buff *skb,
+		     enum ipset_adt adt,
+		     const struct nlmsghdr *nlh,
+		     const struct nlattr * const attr[],
+		     struct netlink_ext_ack *extack)
 {
 	struct ip_set_net *inst = ip_set_pernet(net);
 	struct ip_set *set;
@@ -1593,7 +1595,7 @@ static int ip_set_uadd(struct net *net, struct sock *ctnl, struct sk_buff *skb,
 	if (attr[IPSET_ATTR_DATA]) {
 		if (nla_parse_nested_deprecated(tb, IPSET_ATTR_ADT_MAX, attr[IPSET_ATTR_DATA], set->type->adt_policy, NULL))
 			return -IPSET_ERR_PROTOCOL;
-		ret = call_ad(ctnl, skb, set, tb, IPSET_ADD, flags,
+		ret = call_ad(ctnl, skb, set, tb, adt, flags,
 			      use_lineno);
 	} else {
 		int nla_rem;
@@ -1603,7 +1605,7 @@ static int ip_set_uadd(struct net *net, struct sock *ctnl, struct sk_buff *skb,
 			    !flag_nested(nla) ||
 			    nla_parse_nested_deprecated(tb, IPSET_ATTR_ADT_MAX, nla, set->type->adt_policy, NULL))
 				return -IPSET_ERR_PROTOCOL;
-			ret = call_ad(ctnl, skb, set, tb, IPSET_ADD,
+			ret = call_ad(ctnl, skb, set, tb, adt,
 				      flags, use_lineno);
 			if (ret < 0)
 				return ret;
@@ -1612,55 +1614,22 @@ static int ip_set_uadd(struct net *net, struct sock *ctnl, struct sk_buff *skb,
 	return ret;
 }
 
-static int ip_set_udel(struct net *net, struct sock *ctnl, struct sk_buff *skb,
-		       const struct nlmsghdr *nlh,
+static int ip_set_uadd(struct net *net, struct sock *ctnl,
+		       struct sk_buff *skb, const struct nlmsghdr *nlh,
 		       const struct nlattr * const attr[],
 		       struct netlink_ext_ack *extack)
 {
-	struct ip_set_net *inst = ip_set_pernet(net);
-	struct ip_set *set;
-	struct nlattr *tb[IPSET_ATTR_ADT_MAX + 1] = {};
-	const struct nlattr *nla;
-	u32 flags = flag_exist(nlh);
-	bool use_lineno;
-	int ret = 0;
-
-	if (unlikely(protocol_min_failed(attr) ||
-		     !attr[IPSET_ATTR_SETNAME] ||
-		     !((attr[IPSET_ATTR_DATA] != NULL) ^
-		       (attr[IPSET_ATTR_ADT] != NULL)) ||
-		     (attr[IPSET_ATTR_DATA] &&
-		      !flag_nested(attr[IPSET_ATTR_DATA])) ||
-		     (attr[IPSET_ATTR_ADT] &&
-		      (!flag_nested(attr[IPSET_ATTR_ADT]) ||
-		       !attr[IPSET_ATTR_LINENO]))))
-		return -IPSET_ERR_PROTOCOL;
-
-	set = find_set(inst, nla_data(attr[IPSET_ATTR_SETNAME]));
-	if (!set)
-		return -ENOENT;
-
-	use_lineno = !!attr[IPSET_ATTR_LINENO];
-	if (attr[IPSET_ATTR_DATA]) {
-		if (nla_parse_nested_deprecated(tb, IPSET_ATTR_ADT_MAX, attr[IPSET_ATTR_DATA], set->type->adt_policy, NULL))
-			return -IPSET_ERR_PROTOCOL;
-		ret = call_ad(ctnl, skb, set, tb, IPSET_DEL, flags,
-			      use_lineno);
-	} else {
-		int nla_rem;
+	return ip_set_ad(net, ctnl, skb,
+			 IPSET_ADD, nlh, attr, extack);
+}
 
-		nla_for_each_nested(nla, attr[IPSET_ATTR_ADT], nla_rem) {
-			if (nla_type(nla) != IPSET_ATTR_DATA ||
-			    !flag_nested(nla) ||
-			    nla_parse_nested_deprecated(tb, IPSET_ATTR_ADT_MAX, nla, set->type->adt_policy, NULL))
-				return -IPSET_ERR_PROTOCOL;
-			ret = call_ad(ctnl, skb, set, tb, IPSET_DEL,
-				      flags, use_lineno);
-			if (ret < 0)
-				return ret;
-		}
-	}
-	return ret;
+static int ip_set_udel(struct net *net, struct sock *ctnl,
+		       struct sk_buff *skb, const struct nlmsghdr *nlh,
+		       const struct nlattr * const attr[],
+		       struct netlink_ext_ack *extack)
+{
+	return ip_set_ad(net, ctnl, skb,
+			 IPSET_DEL, nlh, attr, extack);
 }
 
 static int ip_set_utest(struct net *net, struct sock *ctnl, struct sk_buff *skb,
-- 
2.11.0


^ permalink raw reply related

* [PATCH 05/26] netfilter: ipset: Fix the last missing check of nla_parse_deprecated()
From: Pablo Neira Ayuso @ 2019-06-25  0:12 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <20190625001233.22057-1-pablo@netfilter.org>

From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>

In dump_init() the outdated comment was incorrect and we had a missing
validation check of nla_parse_deprecated().

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 net/netfilter/ipset/ip_set_core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index d0f4c627ff91..039892cd2b7d 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1293,11 +1293,13 @@ dump_init(struct netlink_callback *cb, struct ip_set_net *inst)
 	struct nlattr *attr = (void *)nlh + min_len;
 	u32 dump_type;
 	ip_set_id_t index;
+	int ret;
 
-	/* Second pass, so parser can't fail */
-	nla_parse_deprecated(cda, IPSET_ATTR_CMD_MAX, attr,
-			     nlh->nlmsg_len - min_len, ip_set_setname_policy,
-			     NULL);
+	ret = nla_parse_deprecated(cda, IPSET_ATTR_CMD_MAX, attr,
+				   nlh->nlmsg_len - min_len,
+				   ip_set_setname_policy, NULL);
+	if (ret)
+		return ret;
 
 	cb->args[IPSET_CB_PROTO] = nla_get_u8(cda[IPSET_ATTR_PROTOCOL]);
 	if (cda[IPSET_ATTR_SETNAME]) {
-- 
2.11.0


^ permalink raw reply related

* [PATCH 07/26] ipset: Fix memory accounting for hash types on resize
From: Pablo Neira Ayuso @ 2019-06-25  0:12 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <20190625001233.22057-1-pablo@netfilter.org>

From: Stefano Brivio <sbrivio@redhat.com>

If a fresh array block is allocated during resize, the current in-memory
set size should be increased by the size of the block, not replaced by it.

Before the fix, adding entries to a hash set type, leading to a table
resize, caused an inconsistent memory size to be reported. This becomes
more obvious when swapping sets with similar sizes:

  # cat hash_ip_size.sh
  #!/bin/sh
  FAIL_RETRIES=10

  tries=0
  while [ ${tries} -lt ${FAIL_RETRIES} ]; do
  	ipset create t1 hash:ip
  	for i in `seq 1 4345`; do
  		ipset add t1 1.2.$((i / 255)).$((i % 255))
  	done
  	t1_init="$(ipset list t1|sed -n 's/Size in memory: \(.*\)/\1/p')"

  	ipset create t2 hash:ip
  	for i in `seq 1 4360`; do
  		ipset add t2 1.2.$((i / 255)).$((i % 255))
  	done
  	t2_init="$(ipset list t2|sed -n 's/Size in memory: \(.*\)/\1/p')"

  	ipset swap t1 t2
  	t1_swap="$(ipset list t1|sed -n 's/Size in memory: \(.*\)/\1/p')"
  	t2_swap="$(ipset list t2|sed -n 's/Size in memory: \(.*\)/\1/p')"

  	ipset destroy t1
  	ipset destroy t2
  	tries=$((tries + 1))

  	if [ ${t1_init} -lt 10000 ] || [ ${t2_init} -lt 10000 ]; then
  		echo "FAIL after ${tries} tries:"
  		echo "T1 size ${t1_init}, after swap ${t1_swap}"
  		echo "T2 size ${t2_init}, after swap ${t2_swap}"
  		exit 1
  	fi
  done
  echo "PASS"
  # echo -n 'func hash_ip4_resize +p' > /sys/kernel/debug/dynamic_debug/control
  # ./hash_ip_size.sh
  [ 2035.018673] attempt to resize set t1 from 10 to 11, t 00000000fe6551fa
  [ 2035.078583] set t1 resized from 10 (00000000fe6551fa) to 11 (00000000172a0163)
  [ 2035.080353] Table destroy by resize 00000000fe6551fa
  FAIL after 4 tries:
  T1 size 9064, after swap 71128
  T2 size 71128, after swap 9064

Reported-by: NOYB <JunkYardMail1@Frontier.com>
Fixes: 9e41f26a505c ("netfilter: ipset: Count non-static extension memory for userspace")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 net/netfilter/ipset/ip_set_hash_gen.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index 01d51f775f12..623e0d675725 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -625,7 +625,7 @@ mtype_resize(struct ip_set *set, bool retried)
 					goto cleanup;
 				}
 				m->size = AHASH_INIT_SIZE;
-				extsize = ext_size(AHASH_INIT_SIZE, dsize);
+				extsize += ext_size(AHASH_INIT_SIZE, dsize);
 				RCU_INIT_POINTER(hbucket(t, key), m);
 			} else if (m->pos >= m->size) {
 				struct hbucket *ht;
-- 
2.11.0


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox