Netdev List
 help / color / mirror / Atom feed
* [PATCH AUTOSEL 5.2 01/76] batman-adv: Fix netlink dumping of all mcast_flags buckets
From: Sasha Levin @ 2019-08-29 18:11 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Sven Eckelmann, Simon Wunderlich, Sasha Levin, netdev

From: Sven Eckelmann <sven@narfation.org>

[ Upstream commit fa3a03da549a889fc9dbc0d3c5908eb7882cac8f ]

The bucket variable is only updated outside the loop over the mcast_flags
buckets. It will only be updated during a dumping run when the dumping has
to be interrupted and a new message has to be started.

This could result in repeated or missing entries when the multicast flags
are dumped to userspace.

Fixes: d2d489b7d851 ("batman-adv: Add inconsistent multicast netlink dump detection")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 net/batman-adv/multicast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/batman-adv/multicast.c b/net/batman-adv/multicast.c
index ec54e236e3454..50fe9dfb088b6 100644
--- a/net/batman-adv/multicast.c
+++ b/net/batman-adv/multicast.c
@@ -1653,7 +1653,7 @@ __batadv_mcast_flags_dump(struct sk_buff *msg, u32 portid,
 
 	while (bucket_tmp < hash->size) {
 		if (batadv_mcast_flags_dump_bucket(msg, portid, cb, hash,
-						   *bucket, &idx_tmp))
+						   bucket_tmp, &idx_tmp))
 			break;
 
 		bucket_tmp++;
-- 
2.20.1


^ permalink raw reply related

* Re: auto-split of commit. Was: [PATCH bpf-next 04/10] tools/bpf: add libbpf_prog_type_(from|to)_str helpers
From: Arnaldo Carvalho de Melo @ 2019-08-29 18:10 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Greg Kroah-Hartman, Jakub Kicinski, Julia Kartseva, ast,
	Thomas Gleixner, rdna, bpf, daniel, netdev, kernel-team
In-Reply-To: <20190829171655.fww5qxtfusehcpds@ast-mbp.dhcp.thefacebook.com>

Em Thu, Aug 29, 2019 at 10:16:56AM -0700, Alexei Starovoitov escreveu:
> On Thu, Aug 29, 2019 at 08:51:51AM +0200, Greg Kroah-Hartman wrote:
> > That being said, from a "are you keeping the correct authorship info",
> > yes, it sounds like you are doing the correct thing here.

> > Look at what I do for stable kernels, I take the original commit and add
> > it to "another tree" keeping the original author and s-o-b chain intact,
> > and adding a "this is the original git commit id" type message to the
> > changelog text so that people can link it back to the original.
 
> I think you're describing 'git cherry-pick -x'.
> The question was about taking pieces of the original commit. Not the whole commit.
> Author field obviously stays, but SOB is questionable.
> If author meant to change X and Y and Z. Silently taking only Z chunk of the diff
> doesn't quite seem right.
> If we document that such commit split happens in Documentation/bpf/bpf_devel_QA.rst
> do you think it will be enough to properly inform developers?

Can't we instead establish the rule that for something to be added to
tools/include/ it should first land in a separate commit in include/,
ditto for the other things tools/ copies from the kernel sources.

That was the initial intention of tools/include/ and also that is how
tools/perf/check-headers.h works, warning when something ot out of sync,
etc.

I.e. the tools/ maintainers should refuse patches that touch both
tools/include and tools/.

wdyt?

- Arnaldo

^ permalink raw reply

* Re: [PATCH v2 bpf-next 1/3] capability: introduce CAP_BPF and CAP_TRACING
From: Toke Høiland-Jørgensen @ 2019-08-29 18:05 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, luto, davem, peterz, rostedt, netdev, bpf,
	kernel-team, linux-api, brouer
In-Reply-To: <20190829172410.j36gjxt6oku5zh6s@ast-mbp.dhcp.thefacebook.com>

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Thu, Aug 29, 2019 at 09:44:18AM +0200, Toke Høiland-Jørgensen wrote:
>> Alexei Starovoitov <ast@kernel.org> writes:
>> 
>> > CAP_BPF allows the following BPF operations:
>> > - Loading all types of BPF programs
>> > - Creating all types of BPF maps except:
>> >    - stackmap that needs CAP_TRACING
>> >    - devmap that needs CAP_NET_ADMIN
>> >    - cpumap that needs CAP_SYS_ADMIN
>> 
>> Why CAP_SYS_ADMIN instead of CAP_NET_ADMIN for cpumap?
>
> Currently it's cap_sys_admin and I think it should stay this way
> because it creates kthreads.

Ah, right. I can sorta see that makes sense because of the kthreads, but
it also means that you can use all of XDP *except* cpumap with
CAP_NET_ADMIN+CAP_BPF. That is bound to create confusion, isn't it?

-Toke

^ permalink raw reply

* Re: [v1] net_sched: act_police: add 2 new attributes to support police 64bit rate and peakrate
From: Toke Høiland-Jørgensen @ 2019-08-29 17:58 UTC (permalink / raw)
  To: David Z. Dai, Eric Dumazet
  Cc: jhs, xiyou.wangcong, jiri, davem, netdev, linux-kernel, zdai
In-Reply-To: <1567100185.20025.3.camel@oc5348122405>

"David Z. Dai" <zdai@linux.vnet.ibm.com> writes:

> On Thu, 2019-08-29 at 10:32 +0200, Eric Dumazet wrote:
>> 
>> On 8/29/19 12:51 AM, David Dai wrote:
>> > For high speed adapter like Mellanox CX-5 card, it can reach upto
>> > 100 Gbits per second bandwidth. Currently htb already supports 64bit rate
>> > in tc utility. However police action rate and peakrate are still limited
>> > to 32bit value (upto 32 Gbits per second). Add 2 new attributes
>> > TCA_POLICE_RATE64 and TCA_POLICE_RATE64 in kernel for 64bit support
>> > so that tc utility can use them for 64bit rate and peakrate value to
>> > break the 32bit limit, and still keep the backward binary compatibility.
>> > 
>> > Tested-by: David Dai <zdai@linux.vnet.ibm.com>
>> > Signed-off-by: David Dai <zdai@linux.vnet.ibm.com>
>> > ---
>> >  include/uapi/linux/pkt_cls.h |    2 ++
>> >  net/sched/act_police.c       |   27 +++++++++++++++++++++++----
>> >  2 files changed, 25 insertions(+), 4 deletions(-)
>> > 
>> > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>> > index b057aee..eb4ea4d 100644
>> > --- a/include/uapi/linux/pkt_cls.h
>> > +++ b/include/uapi/linux/pkt_cls.h
>> > @@ -159,6 +159,8 @@ enum {
>> >  	TCA_POLICE_AVRATE,
>> >  	TCA_POLICE_RESULT,
>> >  	TCA_POLICE_TM,
>> > +	TCA_POLICE_RATE64,
>> > +	TCA_POLICE_PEAKRATE64,
>> >  	TCA_POLICE_PAD,
>> >  	__TCA_POLICE_MAX
>> >  #define TCA_POLICE_RESULT TCA_POLICE_RESULT
>> 
>> Never insert new attributes, as this breaks compatibility with old binaries (including
>> old kernels)
> Thanks for reviewing it!
> My change is only contained within the police part. I am trying to
> follow the same way htb and tbf support their 64 bit rate.
>
> I tested the old tc binary with the newly patched kernel. It works fine.
>
> I agree the newly compiled tc binary that has these 2 new attributes can
> cause backward compatibility issue when running on the old kernel.
>
> If can't insert new attribute, is there any
> comment/suggestion/alternative on how to support 64bit police rate and
> still keep the backward compatibility?

Just put the new attributes *after* PAD instead of before :)

-Toke

^ permalink raw reply

* Re: [PATCH v3 1/2] net: core: Notify on changes to dev->promiscuity.
From: Ido Schimmel @ 2019-08-29 17:57 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Jiri Pirko, Horatiu Vultur, alexandre.belloni, UNGLinuxDriver,
	davem, allan.nielsen, ivecera, f.fainelli, netdev, linux-kernel
In-Reply-To: <20190829143732.GB17864@lunn.ch>

On Thu, Aug 29, 2019 at 04:37:32PM +0200, Andrew Lunn wrote:
> > Wait, I believe there has been some misundestanding. Promisc mode is NOT
> > about getting packets to the cpu. It's about setting hw filters in a way
> > that no rx packet is dropped.
> > 
> > If you want to get packets from the hw forwarding dataplane to cpu, you
> > should not use promisc mode for that. That would be incorrect.
> 
> Hi Jiri
> 
> I'm not sure a wireshark/tcpdump/pcap user would agree with you. They
> want to see packets on an interface, so they use these tools. The fact
> that the interface is a switch interface should not matter. The
> switchdev model is that we try to hide away the interface happens to
> be on a switch, you can just use it as normal. So why should promisc
> mode not work as normal?

Hi Andrew,

What happens when you run tcpdump on a routed interface without putting
it in promiscuous mode ('-p')? If it is a pure software switch, then you
see all unicast packets addressed to your interface's MAC address. What
happens when the same is done on a hardware switch? With the proposed
solution you will not get the same result.

On a software switch, when you run tcpdump without '-p', do you incur
major packet loss? No. Will this happen when you punt several Tbps to
your CPU on the hardware switch? Yes.

Extending the definition of promiscuous mode to mean punt all traffic to
the CPU is wrong, IMO. You will not be able to capture all the packets
anyway. If you have both elephant and mice flows, then it is very likely
you will not be able to see any packets from the mice flows. The way
this kind of monitoring is usually done is by either sampling packets
(see tc-sample) or mirroring it to capable server. Both options are
available in Linux today.

> > If you want to get packets from the hw forwarding dataplane to cpu, you
> > should use tc trap action. It is there exactly for this purpose.
> 
> Do you really think a wireshark/tcpdump/pcap user should need to use
> tc trap for the special case the interface is a switch port? Doesn't that
> break the switchdev model?

I do not object to the overall goal, but I believe to implementation is
wrong. Instead, it would be much better to extend tshark/tcpdump and
with another flag that will instruct libpcap to install a rule that will
trap all traffic to the CPU. You can do that on either ingress or egress
using matchall and trap action.

If you want to do it without specifying a special flag (I think it's
very dangerous due to the potential packet loss), you can add a flag to
the interface that will indicate to libpcap that installing a tc filter
with trap action is required.

> tc trap is more about fine grained selection of packets.

Depends on the filter you associate with the action. If it's matchall,
then it's not fine grained at all :)

> Also, it seems like trapped packets are not forwarded, which is not
> what you would expect from wireshark/tcpdump/pcap.

How do you mean? Not forwarded by the HW? Right. But the trapped packets
are forwarded by the kernel. We can also add another action that means
both trap and forward. In mlxsw terminology it's called mirror.

^ permalink raw reply

* [PATCH net-next] net: Fail explicit bind to local reserved ports
From: Subash Abhinov Kasiviswanathan @ 2019-08-29  3:26 UTC (permalink / raw)
  To: netdev, davem; +Cc: Subash Abhinov Kasiviswanathan, Sean Tranchetti

Reserved ports may have some special use cases which are not suitable
for use by general userspace applications. Currently, ports specified
in ip_local_reserved_ports will not be returned only in case of
automatic port assignment.

In some cases, it maybe required to prevent the host from assigning
the ports even in case of explicit binds. Consider the case of a
transparent proxy where packets are being redirected. In case a socket
matches this connection, packets from this application would be
incorrectly sent to one of the endpoints.

Add a boolean sysctl flag 'reserved_port_bind'. Default value is 1
which preserves the existing behavior. Setting the value to 0 will
prevent userspace applications from binding to these ports even when
they are explicitly requested.

Cc: Sean Tranchetti <stranche@codeaurora.org>
Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 Documentation/networking/ip-sysctl.txt | 5 +++++
 include/net/netns/ipv4.h               | 2 ++
 net/ipv4/af_inet.c                     | 3 +++
 net/ipv4/inet_connection_sock.c        | 7 +++++++
 net/ipv4/sysctl_net_ipv4.c             | 7 +++++++
 net/ipv4/udp.c                         | 5 +++++
 6 files changed, 29 insertions(+)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 49e95f4..8a9d649 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -945,6 +945,11 @@ ip_unprivileged_port_start - INTEGER
 
 	Default: 1024
 
+reserved_port_bind - BOOLEAN
+	If set, allows explicit bind requests to applications requesting
+	any port within the range of ip_local_reserved_ports.
+	Default: 1
+
 ip_nonlocal_bind - BOOLEAN
 	If set, allows processes to bind() to non-local IP addresses,
 	which can be quite useful - but may break some applications.
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index c0c0791..0941369 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -107,6 +107,8 @@ struct netns_ipv4 {
 #ifdef CONFIG_NET_L3_MASTER_DEV
 	int sysctl_raw_l3mdev_accept;
 #endif
+	int sysctl_reserved_port_bind;
+
 	int sysctl_tcp_early_demux;
 	int sysctl_udp_early_demux;
 
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 70f92aa..e1ad45d 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1814,6 +1814,9 @@ static __net_init int inet_init_net(struct net *net)
 	net->ipv4.ip_local_ports.range[0] =  32768;
 	net->ipv4.ip_local_ports.range[1] =  60999;
 
+	/* Allow explicit binding to reserved ports */
+	net->ipv4.sysctl_reserved_port_bind = 1;
+
 	seqlock_init(&net->ipv4.ping_group_range.lock);
 	/*
 	 * Sane defaults - nobody may create ping sockets.
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index f5c163d..6dda979 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -307,6 +307,13 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 	head = &hinfo->bhash[inet_bhashfn(net, port,
 					  hinfo->bhash_size)];
 	spin_lock_bh(&head->lock);
+
+	if (inet_is_local_reserved_port(net, snum) &&
+	    !net->ipv4.sysctl_reserved_port_bind) {
+		ret = 1;
+		goto fail_unlock;
+	}
+
 	inet_bind_bucket_for_each(tb, &head->chain)
 		if (net_eq(ib_net(tb), net) && tb->l3mdev == l3mdev &&
 		    tb->port == port)
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 59ded25..557fdec 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -742,6 +742,13 @@ static int proc_fib_multipath_hash_policy(struct ctl_table *table, int write,
 		.proc_handler	= proc_do_large_bitmap,
 	},
 	{
+		.procname       = "reserved_port_bind",
+		.data           = &init_net.ipv4.sysctl_reserved_port_bind,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = proc_dointvec
+	},
+	{
 		.procname	= "ip_no_pmtu_disc",
 		.data		= &init_net.ipv4.sysctl_ip_no_pmtu_disc,
 		.maxlen		= sizeof(int),
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index d88821c..59a4274 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -274,6 +274,11 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum,
 	} else {
 		hslot = udp_hashslot(udptable, net, snum);
 		spin_lock_bh(&hslot->lock);
+
+		if (inet_is_local_reserved_port(net, snum) &&
+		    !net->ipv4.sysctl_reserved_port_bind)
+			goto fail_unlock;
+
 		if (hslot->count > 10) {
 			int exist;
 			unsigned int slot2 = udp_sk(sk)->udp_portaddr_hash ^ snum;
-- 
1.9.1


^ permalink raw reply related

* Re: [PATCH] amd-xgbe: Fix error path in xgbe_mod_init()
From: Jakub Kicinski @ 2019-08-29 17:52 UTC (permalink / raw)
  To: YueHaibing; +Cc: thomas.lendacky, davem, netdev, linux-kernel
In-Reply-To: <20190829024600.16052-1-yuehaibing@huawei.com>

On Thu, 29 Aug 2019 10:46:00 +0800, YueHaibing wrote:
> In xgbe_mod_init(), we should do cleanup if some error occurs
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: efbaa828330a ("amd-xgbe: Add support to handle device renaming")
> Fixes: 47f164deab22 ("amd-xgbe: Add PCI device support")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>

Looks correct.

For networking fixes please try to use [PATCH net] as a tag ([PATCH
net-next] for normal, non-fix patches).

^ permalink raw reply

* Re: [PATCH bpf-next] bpf, capabilities: introduce CAP_BPF
From: Steven Rostedt @ 2019-08-29 17:49 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andy Lutomirski, Peter Zijlstra, Alexei Starovoitov, Kees Cook,
	LSM List, James Morris, Jann Horn, Masami Hiramatsu,
	David S. Miller, Daniel Borkmann, Network Development, bpf,
	kernel-team, Linux API
In-Reply-To: <20190829172309.xd73ax4wgsjmv6zg@ast-mbp.dhcp.thefacebook.com>

On Thu, 29 Aug 2019 10:23:10 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> > CAP_TRACE_KERNEL: Use all of perf, ftrace, kprobe, etc.
> > 
> > CAP_TRACE_USER: Use all of perf with scope limited to user mode and uprobes.  
> 
> imo that makes little sense from security pov, since
> such CAP_TRACE_KERNEL (ex kprobe) can trace "unrelated user process"
> just as well. Yet not letting it do cleanly via uprobe.
> Sort of like giving a spare key for back door of the house and
> saying no, you cannot have main door key.

I took it as CAP_TRACE_KERNEL as a superset of CAP_TRACE_USER. That is,
if you have CAP_TRACE_KERNEL, by default you get USER. Where as
CAP_TRACE_USER, is much more limiting.

-- Steve

^ permalink raw reply

* Re: [PATCH bpf-next] bpf, capabilities: introduce CAP_BPF
From: Steven Rostedt @ 2019-08-29 17:47 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Peter Zijlstra, Andy Lutomirski, Alexei Starovoitov, Kees Cook,
	LSM List, James Morris, Jann Horn, Masami Hiramatsu,
	David S. Miller, Daniel Borkmann, Network Development, bpf,
	kernel-team, Linux API
In-Reply-To: <20190829171922.hkuceiurscsxk5jq@ast-mbp.dhcp.thefacebook.com>

On Thu, 29 Aug 2019 10:19:24 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Thu, Aug 29, 2019 at 09:34:34AM -0400, Steven Rostedt wrote:
> > 
> > As the above seems to favor the idea of CAP_TRACING allowing write
> > access to tracefs, should we have a CAP_TRACING_RO for just read access
> > and limited perf abilities?  
> 
> read only vs writeable is an attribute of the file system.
> Bringing such things into caps seem wrong to me.

So using groups then? I'm fine with that.

-- Steve

^ permalink raw reply

* Re: [PATCH bpf-next] bpf, capabilities: introduce CAP_BPF
From: Andy Lutomirski @ 2019-08-29 17:36 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andy Lutomirski, Steven Rostedt, Peter Zijlstra,
	Alexei Starovoitov, Kees Cook, LSM List, James Morris, Jann Horn,
	Masami Hiramatsu, David S. Miller, Daniel Borkmann,
	Network Development, bpf, kernel-team, Linux API
In-Reply-To: <20190829172309.xd73ax4wgsjmv6zg@ast-mbp.dhcp.thefacebook.com>

On Thu, Aug 29, 2019 at 10:23 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Aug 29, 2019 at 08:43:23AM -0700, Andy Lutomirski wrote:
> >
> > I can imagine splitting it into three capabilities:
> >
> > CAP_TRACE_KERNEL: learn which kernel functions are called when.  This
> > would allow perf profiling, for example, but not sampling of kernel
> > regs.
> >
> > CAP_TRACE_READ_KERNEL_DATA: allow the tracing, profiling, etc features
> > that can read the kernel's data.  So you get function arguments via
> > kprobe, kernel regs, and APIs that expose probe_kernel_read()
> >
> > CAP_TRACE_USER: trace unrelated user processes
> >
> > I'm not sure the code is written in a way that makes splitting
> > CAP_TRACE_KERNEL and CAP_TRACE_READ_KERNEL_DATA, and I'm not sure that
> > CAP_TRACE_KERNEL is all that useful except for plain perf record
> > without CAP_TRACE_READ_KERNEL_DATA.  What do you all think?  I suppose
> > it could also be:
> >
> > CAP_PROFILE_KERNEL: Use perf with events that aren't kprobes or
> > tracepoints.  Does not grant the ability to sample regs or the kernel
> > stack directly.
> >
> > CAP_TRACE_KERNEL: Use all of perf, ftrace, kprobe, etc.
> >
> > CAP_TRACE_USER: Use all of perf with scope limited to user mode and uprobes.
>
> imo that makes little sense from security pov, since
> such CAP_TRACE_KERNEL (ex kprobe) can trace "unrelated user process"
> just as well. Yet not letting it do cleanly via uprobe.
> Sort of like giving a spare key for back door of the house and
> saying no, you cannot have main door key.
>

Not all combinations of capabilities make total sense.  CAP_SETUID,
for example, generally lets you get all the other capabilities.
CAP_TRACE_KERNEL + CAP_TRACE_USER makes sense.  CAP_TRACE_USER by
itself makes sense.  CAP_TRACE_READ_KERNEL_DATA without
CAP_TRACE_KERNEL does not.  I don't think this is a really a problem.

^ permalink raw reply

* Re: [v1] net_sched: act_police: add 2 new attributes to support police 64bit rate and peakrate
From: David Z. Dai @ 2019-08-29 17:36 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: jhs, xiyou.wangcong, jiri, davem, netdev, linux-kernel, zdai
In-Reply-To: <7a8a5024-bbff-7443-71b3-9e3976af269f@gmail.com>

On Thu, 2019-08-29 at 10:32 +0200, Eric Dumazet wrote:
> 
> On 8/29/19 12:51 AM, David Dai wrote:
> > For high speed adapter like Mellanox CX-5 card, it can reach upto
> > 100 Gbits per second bandwidth. Currently htb already supports 64bit rate
> > in tc utility. However police action rate and peakrate are still limited
> > to 32bit value (upto 32 Gbits per second). Add 2 new attributes
> > TCA_POLICE_RATE64 and TCA_POLICE_RATE64 in kernel for 64bit support
> > so that tc utility can use them for 64bit rate and peakrate value to
> > break the 32bit limit, and still keep the backward binary compatibility.
> > 
> > Tested-by: David Dai <zdai@linux.vnet.ibm.com>
> > Signed-off-by: David Dai <zdai@linux.vnet.ibm.com>
> > ---
> >  include/uapi/linux/pkt_cls.h |    2 ++
> >  net/sched/act_police.c       |   27 +++++++++++++++++++++++----
> >  2 files changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> > index b057aee..eb4ea4d 100644
> > --- a/include/uapi/linux/pkt_cls.h
> > +++ b/include/uapi/linux/pkt_cls.h
> > @@ -159,6 +159,8 @@ enum {
> >  	TCA_POLICE_AVRATE,
> >  	TCA_POLICE_RESULT,
> >  	TCA_POLICE_TM,
> > +	TCA_POLICE_RATE64,
> > +	TCA_POLICE_PEAKRATE64,
> >  	TCA_POLICE_PAD,
> >  	__TCA_POLICE_MAX
> >  #define TCA_POLICE_RESULT TCA_POLICE_RESULT
> 
> Never insert new attributes, as this breaks compatibility with old binaries (including
> old kernels)
Thanks for reviewing it!
My change is only contained within the police part. I am trying to
follow the same way htb and tbf support their 64 bit rate.

I tested the old tc binary with the newly patched kernel. It works fine.

I agree the newly compiled tc binary that has these 2 new attributes can
cause backward compatibility issue when running on the old kernel.

If can't insert new attribute, is there any
comment/suggestion/alternative on how to support 64bit police rate and
still keep the backward compatibility?

> Keep TCA_POLICE_PAD value the same, thanks.



^ permalink raw reply

* Re: [PATCH v2 bpf-next 2/3] bpf: implement CAP_BPF
From: Alexei Starovoitov @ 2019-08-29 17:30 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Alexei Starovoitov, luto, davem, peterz, rostedt, netdev, bpf,
	kernel-team, linux-api
In-Reply-To: <ed8796f5-eaea-c87d-ddd9-9d624059e5ee@iogearbox.net>

On Thu, Aug 29, 2019 at 05:32:27PM +0200, Daniel Borkmann wrote:
> On 8/29/19 7:12 AM, Alexei Starovoitov wrote:
> > Implement permissions as stated in uapi/linux/capability.h
> > 
> > Note that CAP_SYS_ADMIN is replaced with CAP_BPF.
> > All existing applications that use BPF do not drop all caps
> > and keep only CAP_SYS_ADMIN before doing bpf() syscall.
> > Hence it's highly unlikely that existing code will break.
> > If there will be reports of breakage then CAP_SYS_ADMIN
> > would be allowed as well with "it's usage is deprecated" message
> > similar to commit ee24aebffb75 ("cap_syslog: accept CAP_SYS_ADMIN for now")
> > 
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> [...]
> > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > index 22066a62c8c9..f459315625ac 100644
> > --- a/kernel/bpf/hashtab.c
> > +++ b/kernel/bpf/hashtab.c
> > @@ -244,9 +244,9 @@ static int htab_map_alloc_check(union bpf_attr *attr)
> >   	BUILD_BUG_ON(offsetof(struct htab_elem, fnode.next) !=
> >   		     offsetof(struct htab_elem, hash_node.pprev));
> > -	if (lru && !capable(CAP_SYS_ADMIN))
> > +	if (lru && !capable(CAP_BPF))
> >   		/* LRU implementation is much complicated than other
> > -		 * maps.  Hence, limit to CAP_SYS_ADMIN for now.
> > +		 * maps.  Hence, limit to CAP_BPF.
> >   		 */
> >   		return -EPERM;
> I don't think this works, this is pretty much going to break use cases where
> orchestration daemons are deployed as containers that are explicitly granted
> specified cap set and right now this is CAP_SYS_ADMIN and not CAP_BPF for bpf().
> The former needs to be a superset of the latter in order for this to work and
> not break compatibility between kernel upgrades.
> 
> - https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-capabilities-for-a-container
> - https://docs.docker.com/engine/reference/run/#runtime-privilege-and-linux-capabilities

These are the links that showing that k8 can delegates caps.
Are you saying that you know of folks who specifically
delegate cap_sys_admin and cap_net_admin _only_ to a container to run bpf in there?


^ permalink raw reply

* Re: [PATCH v2 bpf-next 2/3] bpf: implement CAP_BPF
From: Alexei Starovoitov @ 2019-08-29 17:28 UTC (permalink / raw)
  To: Song Liu
  Cc: Alexei Starovoitov, Andy Lutomirski, davem@davemloft.net,
	peterz@infradead.org, rostedt@goodmis.org, netdev@vger.kernel.org,
	bpf@vger.kernel.org, Kernel Team, linux-api@vger.kernel.org
In-Reply-To: <B28631A9-BB92-404A-BD58-7A737BCF10C9@fb.com>

On Thu, Aug 29, 2019 at 06:04:42AM +0000, Song Liu wrote:
> 
> 
> > On Aug 28, 2019, at 10:12 PM, Alexei Starovoitov <ast@kernel.org> wrote:
> > 
> 
> [...]
> 
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index 44e2d640b088..91a7f25512ca 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -805,10 +805,20 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> > 	}
> > }
> > 
> > +struct libcap {
> > +	struct __user_cap_header_struct hdr;
> > +	struct __user_cap_data_struct data[2];
> > +};
> > +
> 
> I am confused by struct libcap. Why do we need it? 

because libcap is not compatible with new kernel.
It needs to be recompiled with new capability.h
Otherwise it limits max to CAP_AUDIT_READ
Any value higher it will error during cap_get_flag.
And will silently ignore it during cap_set_flag.
Not a great library decision.

Thankfully this struct above is exactly the kernel api.
One doesn't really need libcap. It's imo easier to do without it.


^ permalink raw reply

* Re: [PATCH v2 2/2] PTP: add support for one-shot output
From: Richard Cochran @ 2019-08-29 17:28 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel, davem
In-Reply-To: <20190829172509.GB2166@localhost>


Adding davem onto CC...

On Thu, Aug 29, 2019 at 12:58:25PM +0300, Felipe Balbi wrote:
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 98ec1395544e..a407e5f76e2d 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -177,9 +177,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			err = -EFAULT;
>  			break;
>  		}
> -		if ((req.perout.flags || req.perout.rsv[0] || req.perout.rsv[1]
> -				|| req.perout.rsv[2] || req.perout.rsv[3])
> -			&& cmd == PTP_PEROUT_REQUEST2) {
> +		if ((req.perout.rsv[0] || req.perout.rsv[1] || req.perout.rsv[2]
> +			|| req.perout.rsv[3]) && cmd == PTP_PEROUT_REQUEST2) {

Please check that the reserved bits of req.perout.flags, namely
~PTP_PEROUT_ONE_SHOT, are clear.

>  			err = -EINVAL;
>  			break;
>  		} else if (cmd == PTP_PEROUT_REQUEST) {
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 039cd62ec706..95840e5f5c53 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -67,7 +67,9 @@ struct ptp_perout_request {
>  	struct ptp_clock_time start;  /* Absolute start time. */
>  	struct ptp_clock_time period; /* Desired period, zero means disable. */
>  	unsigned int index;           /* Which channel to configure. */
> -	unsigned int flags;           /* Reserved for future use. */
> +
> +#define PTP_PEROUT_ONE_SHOT BIT(0)
> +	unsigned int flags;

@davem  Any CodingStyle policy on #define within a struct?  (Some
maintainers won't allow it.)

>  	unsigned int rsv[4];          /* Reserved for future use. */
>  };
>  
> -- 
> 2.23.0
> 

Thanks,
Richard

^ permalink raw reply

* Re: [PATCH bpf-next 0/3] tools: bpftool: improve bpftool build experience
From: Jakub Kicinski @ 2019-08-29 17:25 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers,
	Lorenz Bauer, Ilya Leoshkevich
In-Reply-To: <20190829105645.12285-1-quentin.monnet@netronome.com>

On Thu, 29 Aug 2019 11:56:42 +0100, Quentin Monnet wrote:
> Hi,
> This set attempts to make it easier to build bpftool, in particular when
> passing a specific output directory. This is a follow-up to the
> conversation held last month by Lorenz, Ilya and Jakub [0].
> 
> The first patch is a minor fix to bpftool's Makefile, regarding the
> retrieval of kernel version (which currently prints a non-relevant make
> warning on some invocations).
> 
> Second patch improves the Makefile commands to support more "make"
> invocations, or to fix building with custom output directory. On Jakub's
> suggestion, a script is also added to BPF selftests in order to keep track
> of the supported build variants.
> 
> At last, third patch is a sligthly modified version of Ilya's fix regarding
> libbpf.a appearing twice on the linking command for bpftool.
> 
> [0] https://lore.kernel.org/bpf/CACAyw9-CWRHVH3TJ=Tke2x8YiLsH47sLCijdp=V+5M836R9aAA@mail.gmail.com/

I think Ilya has a point, but otherwise looks good to me :)

Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

^ permalink raw reply

* Re: [PATCH v2 bpf-next 1/3] capability: introduce CAP_BPF and CAP_TRACING
From: Alexei Starovoitov @ 2019-08-29 17:25 UTC (permalink / raw)
  To: Nicolas Dichtel
  Cc: Alexei Starovoitov, luto, davem, peterz, rostedt, netdev, bpf,
	kernel-team, linux-api
In-Reply-To: <a5ef2f94-acca-eb66-b48c-899494a9f8d0@6wind.com>

On Thu, Aug 29, 2019 at 03:36:42PM +0200, Nicolas Dichtel wrote:
> Le 29/08/2019 à 07:12, Alexei Starovoitov a écrit :
> [snip]
> > CAP_BPF and CAP_NET_ADMIN together allow the following:
> > - Attach to cgroup-bpf hooks and query
> > - skb, xdp, flow_dissector test_run command
> > 
> > CAP_NET_ADMIN allows:
> > - Attach networking bpf programs to xdp, tc, lwt, flow dissector
> I'm not sure to understand the difference between these last two points.
> But, with the current kernel, CAP_NET_ADMIN is not enough to attach bpf prog
> with tc and it's still not enough after your patch.
> The following command is rejected:
> $ tc filter add dev eth0 ingress matchall action bpf obj ./tc_test_kern.o sec test
> 
> Prog section 'test' rejected: Operation not permitted (1)!
>  - Type:         4
>  - Instructions: 22 (0 over limit)
>  - License:      GPL
> 
> Verifier analysis:
> 
> Error fetching program/map!
> bad action parsing
> parse_action: bad value (5:bpf)!
> Illegal "action"

because tc/iproute2 is doing load and attach.
Currently load needs cap_sys_admin and
attach needs cap_net_admin.


^ permalink raw reply

* Re: [PATCH v2 2/2] PTP: add support for one-shot output
From: Richard Cochran @ 2019-08-29 17:25 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel
In-Reply-To: <20190829095825.2108-2-felipe.balbi@linux.intel.com>

On Thu, Aug 29, 2019 at 12:58:25PM +0300, Felipe Balbi wrote:
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 98ec1395544e..a407e5f76e2d 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -177,9 +177,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			err = -EFAULT;
>  			break;
>  		}
> -		if ((req.perout.flags || req.perout.rsv[0] || req.perout.rsv[1]
> -				|| req.perout.rsv[2] || req.perout.rsv[3])
> -			&& cmd == PTP_PEROUT_REQUEST2) {
> +		if ((req.perout.rsv[0] || req.perout.rsv[1] || req.perout.rsv[2]
> +			|| req.perout.rsv[3]) && cmd == PTP_PEROUT_REQUEST2) {

Please check that the reserved bits of req.perout.flags, namely
~PTP_PEROUT_ONE_SHOT, are clear.

>  			err = -EINVAL;
>  			break;
>  		} else if (cmd == PTP_PEROUT_REQUEST) {
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 039cd62ec706..95840e5f5c53 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -67,7 +67,9 @@ struct ptp_perout_request {
>  	struct ptp_clock_time start;  /* Absolute start time. */
>  	struct ptp_clock_time period; /* Desired period, zero means disable. */
>  	unsigned int index;           /* Which channel to configure. */
> -	unsigned int flags;           /* Reserved for future use. */
> +
> +#define PTP_PEROUT_ONE_SHOT BIT(0)
> +	unsigned int flags;

@davem  Any CodingStyle policy on #define within a struct?  (Some
maintainers won't allow it.)

>  	unsigned int rsv[4];          /* Reserved for future use. */
>  };
>  
> -- 
> 2.23.0
> 

Thanks,
Richard

^ permalink raw reply

* Re: [PATCH v2 bpf-next 1/3] capability: introduce CAP_BPF and CAP_TRACING
From: Alexei Starovoitov @ 2019-08-29 17:24 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Alexei Starovoitov, luto, davem, peterz, rostedt, netdev, bpf,
	kernel-team, linux-api
In-Reply-To: <87ef14iffx.fsf@toke.dk>

On Thu, Aug 29, 2019 at 09:44:18AM +0200, Toke Høiland-Jørgensen wrote:
> Alexei Starovoitov <ast@kernel.org> writes:
> 
> > CAP_BPF allows the following BPF operations:
> > - Loading all types of BPF programs
> > - Creating all types of BPF maps except:
> >    - stackmap that needs CAP_TRACING
> >    - devmap that needs CAP_NET_ADMIN
> >    - cpumap that needs CAP_SYS_ADMIN
> 
> Why CAP_SYS_ADMIN instead of CAP_NET_ADMIN for cpumap?

Currently it's cap_sys_admin and I think it should stay this way
because it creates kthreads.


^ permalink raw reply

* Re: [PATCH bpf-next] bpf, capabilities: introduce CAP_BPF
From: Alexei Starovoitov @ 2019-08-29 17:23 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Steven Rostedt, Peter Zijlstra, Alexei Starovoitov, Kees Cook,
	LSM List, James Morris, Jann Horn, Masami Hiramatsu,
	David S. Miller, Daniel Borkmann, Network Development, bpf,
	kernel-team, Linux API
In-Reply-To: <CALCETrWYu0XB_d-MhXFgopEmBu-pog493G1e+KsE3dS32UULgA@mail.gmail.com>

On Thu, Aug 29, 2019 at 08:43:23AM -0700, Andy Lutomirski wrote:
> 
> I can imagine splitting it into three capabilities:
> 
> CAP_TRACE_KERNEL: learn which kernel functions are called when.  This
> would allow perf profiling, for example, but not sampling of kernel
> regs.
> 
> CAP_TRACE_READ_KERNEL_DATA: allow the tracing, profiling, etc features
> that can read the kernel's data.  So you get function arguments via
> kprobe, kernel regs, and APIs that expose probe_kernel_read()
> 
> CAP_TRACE_USER: trace unrelated user processes
> 
> I'm not sure the code is written in a way that makes splitting
> CAP_TRACE_KERNEL and CAP_TRACE_READ_KERNEL_DATA, and I'm not sure that
> CAP_TRACE_KERNEL is all that useful except for plain perf record
> without CAP_TRACE_READ_KERNEL_DATA.  What do you all think?  I suppose
> it could also be:
> 
> CAP_PROFILE_KERNEL: Use perf with events that aren't kprobes or
> tracepoints.  Does not grant the ability to sample regs or the kernel
> stack directly.
> 
> CAP_TRACE_KERNEL: Use all of perf, ftrace, kprobe, etc.
> 
> CAP_TRACE_USER: Use all of perf with scope limited to user mode and uprobes.

imo that makes little sense from security pov, since
such CAP_TRACE_KERNEL (ex kprobe) can trace "unrelated user process"
just as well. Yet not letting it do cleanly via uprobe.
Sort of like giving a spare key for back door of the house and
saying no, you cannot have main door key.


^ permalink raw reply

* Re: [PATCH v2 1/2] PTP: introduce new versions of IOCTLs
From: Richard Cochran @ 2019-08-29 17:21 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel
In-Reply-To: <20190829095825.2108-1-felipe.balbi@linux.intel.com>

> @@ -139,11 +141,24 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  		break;
>  
>  	case PTP_EXTTS_REQUEST:
> +	case PTP_EXTTS_REQUEST2:
> +		memset(&req, 0, sizeof(req));
> +
>  		if (copy_from_user(&req.extts, (void __user *)arg,
>  				   sizeof(req.extts))) {
>  			err = -EFAULT;
>  			break;
>  		}
> +		if ((req.extts.flags || req.extts.rsv[0] || req.extts.rsv[1])
> +			&& cmd == PTP_EXTTS_REQUEST2) {
> +			err = -EINVAL;
> +			break;
> +		} else if (cmd == PTP_EXTTS_REQUEST) {
> +			req.extts.flags = 0;

This still isn't quite right.  Sorry that was my fault.

The req.extts.flags can be (PTP_ENABLE_FEATURE | PTP_RISING_EDGE |
PTP_FALLING_EDGE), and ENABLE is used immediately below in this case.

Please #define those bits into a valid mask, and then:

- for PTP_EXTTS_REQUEST2 check that ~mask is zero, and
- for PTP_EXTTS_REQUEST clear the ~mask bits for the drivers. 

Thanks again for cleaning this up!

Richard

> +			req.extts.rsv[0] = 0;
> +			req.extts.rsv[1] = 0;
> +		}
> +			
>  		if (req.extts.index >= ops->n_ext_ts) {
>  			err = -EINVAL;
>  			break;

^ permalink raw reply

* Re: [PATCH bpf-next] bpf, capabilities: introduce CAP_BPF
From: Alexei Starovoitov @ 2019-08-29 17:19 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Peter Zijlstra, Andy Lutomirski, Alexei Starovoitov, Kees Cook,
	LSM List, James Morris, Jann Horn, Masami Hiramatsu,
	David S. Miller, Daniel Borkmann, Network Development, bpf,
	kernel-team, Linux API
In-Reply-To: <20190829093434.36540972@gandalf.local.home>

On Thu, Aug 29, 2019 at 09:34:34AM -0400, Steven Rostedt wrote:
> 
> As the above seems to favor the idea of CAP_TRACING allowing write
> access to tracefs, should we have a CAP_TRACING_RO for just read access
> and limited perf abilities?

read only vs writeable is an attribute of the file system.
Bringing such things into caps seem wrong to me.


^ permalink raw reply

* Re: auto-split of commit. Was: [PATCH bpf-next 04/10] tools/bpf: add libbpf_prog_type_(from|to)_str helpers
From: Alexei Starovoitov @ 2019-08-29 17:16 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jakub Kicinski, Julia Kartseva, ast, Thomas Gleixner, rdna, bpf,
	daniel, netdev, kernel-team
In-Reply-To: <20190829065151.GB30423@kroah.com>

On Thu, Aug 29, 2019 at 08:51:51AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 28, 2019 at 04:46:28PM -0700, Alexei Starovoitov wrote:
> > On Wed, Aug 28, 2019 at 04:34:22PM -0700, Jakub Kicinski wrote:
> > > 
> > > Greg, Thomas, libbpf is extracted from the kernel sources and
> > > maintained in a clone repo on GitHub for ease of packaging.
> > > 
> > > IIUC Alexei's concern is that since we are moving the commits from
> > > the kernel repo to the GitHub one we have to preserve the commits
> > > exactly as they are, otherwise SOB lines lose their power.
> > > 
> > > Can you provide some guidance on whether that's a valid concern, 
> > > or whether it's perfectly fine to apply a partial patch?
> > 
> > Right. That's exactly the concern.
> > 
> > Greg, Thomas,
> > could you please put your legal hat on and clarify the following.
> > Say some developer does a patch that modifies
> > include/uapi/linux/bpf.h
> > ..some other kernel code...and
> > tools/include/uapi/linux/bpf.h
> > 
> > That tools/include/uapi/linux/bpf.h is used by perf and by libbpf.
> > We have automatic mirror of tools/libbpf into github/libbpf/
> > so that external projects and can do git submodule of it,
> > can build packages out of it, etc.
> > 
> > The question is whether it's ok to split tools/* part out of
> > original commit, keep Author and SOB, create new commit out of it,
> > and automatically push that auto-generated commit into github mirror.
> 
> Note, I am not a laywer, and am not _your_ lawyer either, only _your_
> lawyer can answer questions as to what is best for you.
> 
> That being said, from a "are you keeping the correct authorship info",
> yes, it sounds like you are doing the correct thing here.
> 
> Look at what I do for stable kernels, I take the original commit and add
> it to "another tree" keeping the original author and s-o-b chain intact,
> and adding a "this is the original git commit id" type message to the
> changelog text so that people can link it back to the original.

I think you're describing 'git cherry-pick -x'.
The question was about taking pieces of the original commit. Not the whole commit.
Author field obviously stays, but SOB is questionable.
If author meant to change X and Y and Z. Silently taking only Z chunk of the diff
doesn't quite seem right.
If we document that such commit split happens in Documentation/bpf/bpf_devel_QA.rst
do you think it will be enough to properly inform developers?
The main concern is the surprise factor when people start seeing their commits
in the mirror, but not their full commits.


^ permalink raw reply

* Re: [PATCH net-next] net: dsa: mv88e6xxx: fix freeing unused SERDES IRQ
From: Andrew Lunn @ 2019-08-29 17:08 UTC (permalink / raw)
  To: Vivien Didelot; +Cc: netdev, davem, Marek Behún, f.fainelli
In-Reply-To: <20190828185511.21956-1-vivien.didelot@gmail.com>

On Wed, Aug 28, 2019 at 02:55:11PM -0400, Vivien Didelot wrote:
> Now mv88e6xxx does not enable its ports at setup itself and let
> the DSA core handle this, unused ports are disabled without being
> powered on first. While that is expected, the SERDES powering code
> was assuming that a port was already set up before powering it down,
> resulting in freeing an unused IRQ. The patch fixes this assumption.
> 
> Fixes: b759f528ca3d ("net: dsa: mv88e6xxx: enable SERDES after setup")
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>

Tested-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply

* [PATCH net-next v2 3/3] net: tls: export protocol version, cipher, tx_conf/rx_conf to socket diag
From: Davide Caratti @ 2019-08-29 16:48 UTC (permalink / raw)
  To: borisp, jakub.kicinski, Eric Dumazet
  Cc: aviadye, davejwatson, davem, john.fastabend, Matthieu Baerts,
	netdev
In-Reply-To: <cover.1567095873.git.dcaratti@redhat.com>

When an application configures kernel TLS on top of a TCP socket, it's
now possible for inet_diag_handler() to collect information regarding the
protocol version, the cipher type and TX / RX configuration, in case
INET_DIAG_INFO is requested.

Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 include/net/tls.h              | 19 ++++++++++
 include/uapi/linux/inet_diag.h |  1 +
 include/uapi/linux/tls.h       | 15 ++++++++
 net/tls/tls_main.c             | 64 ++++++++++++++++++++++++++++++++++
 4 files changed, 99 insertions(+)

diff --git a/include/net/tls.h b/include/net/tls.h
index 4997742475cd..990f1d9182a3 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -431,6 +431,25 @@ static inline bool is_tx_ready(struct tls_sw_context_tx *ctx)
 	return READ_ONCE(rec->tx_ready);
 }
 
+static inline u16 tls_user_config(struct tls_context *ctx, bool tx)
+{
+	u16 config = tx ? ctx->tx_conf : ctx->rx_conf;
+
+	switch (config) {
+	case TLS_BASE:
+		return TLS_CONF_BASE;
+	case TLS_SW:
+		return TLS_CONF_SW;
+#ifdef CONFIG_TLS_DEVICE
+	case TLS_HW:
+		return TLS_CONF_HW;
+#endif
+	case TLS_HW_RECORD:
+		return TLS_CONF_HW_RECORD;
+	}
+	return 0;
+}
+
 struct sk_buff *
 tls_validate_xmit_skb(struct sock *sk, struct net_device *dev,
 		      struct sk_buff *skb);
diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
index e2c6273274f3..a1ff345b3f33 100644
--- a/include/uapi/linux/inet_diag.h
+++ b/include/uapi/linux/inet_diag.h
@@ -162,6 +162,7 @@ enum {
 enum {
 	INET_ULP_INFO_UNSPEC,
 	INET_ULP_INFO_NAME,
+	INET_ULP_INFO_TLS,
 	__INET_ULP_INFO_MAX,
 };
 #define INET_ULP_INFO_MAX (__INET_ULP_INFO_MAX - 1)
diff --git a/include/uapi/linux/tls.h b/include/uapi/linux/tls.h
index 5b9c26753e46..bcd2869ed472 100644
--- a/include/uapi/linux/tls.h
+++ b/include/uapi/linux/tls.h
@@ -109,4 +109,19 @@ struct tls12_crypto_info_aes_ccm_128 {
 	unsigned char rec_seq[TLS_CIPHER_AES_CCM_128_REC_SEQ_SIZE];
 };
 
+enum {
+	TLS_INFO_UNSPEC,
+	TLS_INFO_VERSION,
+	TLS_INFO_CIPHER,
+	TLS_INFO_TXCONF,
+	TLS_INFO_RXCONF,
+	__TLS_INFO_MAX,
+};
+#define TLS_INFO_MAX (__TLS_INFO_MAX - 1)
+
+#define TLS_CONF_BASE 1
+#define TLS_CONF_SW 2
+#define TLS_CONF_HW 3
+#define TLS_CONF_HW_RECORD 4
+
 #endif /* _UAPI_LINUX_TLS_H */
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index f8f2d2c3d627..3351a2ace369 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -39,6 +39,7 @@
 #include <linux/netdevice.h>
 #include <linux/sched/signal.h>
 #include <linux/inetdevice.h>
+#include <linux/inet_diag.h>
 
 #include <net/tls.h>
 
@@ -835,6 +836,67 @@ static void tls_update(struct sock *sk, struct proto *p)
 	}
 }
 
+static int tls_get_info(const struct sock *sk, struct sk_buff *skb)
+{
+	struct tls_context *ctx;
+	u16 version, cipher_type;
+	struct nlattr *start;
+	int err;
+
+	start = nla_nest_start_noflag(skb, INET_ULP_INFO_TLS);
+	if (!start)
+		return -EMSGSIZE;
+
+	rcu_read_lock();
+	ctx = rcu_dereference(inet_csk(sk)->icsk_ulp_data);
+	if (!ctx) {
+		err = 0;
+		goto nla_failure;
+	}
+	version = ctx->prot_info.version;
+	if (version) {
+		err = nla_put_u16(skb, TLS_INFO_VERSION, version);
+		if (err)
+			goto nla_failure;
+	}
+	cipher_type = ctx->prot_info.cipher_type;
+	if (cipher_type) {
+		err = nla_put_u16(skb, TLS_INFO_CIPHER, cipher_type);
+		if (err)
+			goto nla_failure;
+	}
+	err = nla_put_u16(skb, TLS_INFO_TXCONF, tls_user_config(ctx, true));
+	if (err)
+		goto nla_failure;
+
+	err = nla_put_u16(skb, TLS_INFO_RXCONF, tls_user_config(ctx, false));
+	if (err)
+		goto nla_failure;
+
+	rcu_read_unlock();
+	nla_nest_end(skb, start);
+	return 0;
+
+nla_failure:
+	rcu_read_unlock();
+	nla_nest_cancel(skb, start);
+	return err;
+}
+
+static size_t tls_get_info_size(const struct sock *sk)
+{
+	size_t size = 0;
+
+	size += nla_total_size(0) +		/* INET_ULP_INFO_TLS */
+		nla_total_size(sizeof(u16)) +	/* TLS_INFO_VERSION */
+		nla_total_size(sizeof(u16)) +	/* TLS_INFO_CIPHER */
+		nla_total_size(sizeof(u16)) +	/* TLS_INFO_RXCONF */
+		nla_total_size(sizeof(u16)) +	/* TLS_INFO_TXCONF */
+		0;
+
+	return size;
+}
+
 void tls_register_device(struct tls_device *device)
 {
 	spin_lock_bh(&device_spinlock);
@@ -856,6 +918,8 @@ static struct tcp_ulp_ops tcp_tls_ulp_ops __read_mostly = {
 	.owner			= THIS_MODULE,
 	.init			= tls_init,
 	.update			= tls_update,
+	.get_info		= tls_get_info,
+	.get_info_size		= tls_get_info_size,
 };
 
 static int __init tls_register(void)
-- 
2.20.1


^ permalink raw reply related

* [PATCH net-next v2 2/3] tcp: ulp: add functions to dump ulp-specific information
From: Davide Caratti @ 2019-08-29 16:48 UTC (permalink / raw)
  To: borisp, jakub.kicinski, Eric Dumazet
  Cc: aviadye, davejwatson, davem, john.fastabend, Matthieu Baerts,
	netdev
In-Reply-To: <cover.1567095873.git.dcaratti@redhat.com>

currently, only getsockopt(TCP_ULP) can be invoked to know if a ULP is on
top of a TCP socket. Extend idiag_get_aux() and idiag_get_aux_size(),
introduced by commit b37e88407c1d ("inet_diag: allow protocols to provide
additional data"), to report the ULP name and other information that can
be made available by the ULP through optional functions.

Users having CAP_NET_ADMIN privileges will then be able to retrieve this
information through inet_diag_handler, if they specify INET_DIAG_INFO in
the request.

Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 include/net/tcp.h              |  3 ++
 include/uapi/linux/inet_diag.h |  8 ++++++
 net/ipv4/tcp_diag.c            | 52 +++++++++++++++++++++++++++++++++-
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 77fe87f7a992..c9a3f9688223 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2122,6 +2122,9 @@ struct tcp_ulp_ops {
 	void (*update)(struct sock *sk, struct proto *p);
 	/* cleanup ulp */
 	void (*release)(struct sock *sk);
+	/* diagnostic */
+	int (*get_info)(const struct sock *sk, struct sk_buff *skb);
+	size_t (*get_info_size)(const struct sock *sk);
 
 	char		name[TCP_ULP_NAME_MAX];
 	struct module	*owner;
diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
index e8baca85bac6..e2c6273274f3 100644
--- a/include/uapi/linux/inet_diag.h
+++ b/include/uapi/linux/inet_diag.h
@@ -153,11 +153,19 @@ enum {
 	INET_DIAG_BBRINFO,	/* request as INET_DIAG_VEGASINFO */
 	INET_DIAG_CLASS_ID,	/* request as INET_DIAG_TCLASS */
 	INET_DIAG_MD5SIG,
+	INET_DIAG_ULP_INFO,
 	__INET_DIAG_MAX,
 };
 
 #define INET_DIAG_MAX (__INET_DIAG_MAX - 1)
 
+enum {
+	INET_ULP_INFO_UNSPEC,
+	INET_ULP_INFO_NAME,
+	__INET_ULP_INFO_MAX,
+};
+#define INET_ULP_INFO_MAX (__INET_ULP_INFO_MAX - 1)
+
 /* INET_DIAG_MEM */
 
 struct inet_diag_meminfo {
diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
index a3a386236d93..babc156deabb 100644
--- a/net/ipv4/tcp_diag.c
+++ b/net/ipv4/tcp_diag.c
@@ -81,13 +81,42 @@ static int tcp_diag_put_md5sig(struct sk_buff *skb,
 }
 #endif
 
+static int tcp_diag_put_ulp(struct sk_buff *skb, struct sock *sk,
+			    const struct tcp_ulp_ops *ulp_ops)
+{
+	struct nlattr *nest;
+	int err;
+
+	nest = nla_nest_start_noflag(skb, INET_DIAG_ULP_INFO);
+	if (!nest)
+		return -EMSGSIZE;
+
+	err = nla_put_string(skb, INET_ULP_INFO_NAME, ulp_ops->name);
+	if (err)
+		goto nla_failure;
+
+	if (ulp_ops->get_info)
+		err = ulp_ops->get_info(sk, skb);
+	if (err)
+		goto nla_failure;
+
+	nla_nest_end(skb, nest);
+	return 0;
+
+nla_failure:
+	nla_nest_cancel(skb, nest);
+	return err;
+}
+
 static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
 			    struct sk_buff *skb)
 {
+	struct inet_connection_sock *icsk = inet_csk(sk);
+	int err = 0;
+
 #ifdef CONFIG_TCP_MD5SIG
 	if (net_admin) {
 		struct tcp_md5sig_info *md5sig;
-		int err = 0;
 
 		rcu_read_lock();
 		md5sig = rcu_dereference(tcp_sk(sk)->md5sig_info);
@@ -99,11 +128,21 @@ static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
 	}
 #endif
 
+	if (net_admin) {
+		const struct tcp_ulp_ops *ulp_ops;
+
+		ulp_ops = icsk->icsk_ulp_ops;
+		if (ulp_ops)
+			err = tcp_diag_put_ulp(skb, sk, ulp_ops);
+		if (err)
+			return err;
+	}
 	return 0;
 }
 
 static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
 {
+	struct inet_connection_sock *icsk = inet_csk(sk);
 	size_t size = 0;
 
 #ifdef CONFIG_TCP_MD5SIG
@@ -124,6 +163,17 @@ static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
 	}
 #endif
 
+	if (net_admin) {
+		const struct tcp_ulp_ops *ulp_ops;
+
+		ulp_ops = icsk->icsk_ulp_ops;
+		if (ulp_ops) {
+			size += nla_total_size(0) +
+				nla_total_size(TCP_ULP_NAME_MAX);
+			if (ulp_ops->get_info_size)
+				size += ulp_ops->get_info_size(sk);
+		}
+	}
 	return size;
 }
 
-- 
2.20.1


^ 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