* Re: [PATCH] Documentation: net: phy: improve explanation when to specify the PHY ID
From: David Miller @ 2017-01-24 18:31 UTC (permalink / raw)
To: martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg
Cc: andrew-g2DYL2Zd6BY, devicetree-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, f.fainelli-Re5JQEeQqe8AvxtiuMwx3w,
mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20170122164132.13967-1-martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
From: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Date: Sun, 22 Jan 2017 17:41:32 +0100
> The old description basically read like "ethernet-phy-idAAAA.BBBB" can
> be specified when you know the actual PHY ID. However, specifying this
> has a side-effect: it forces Linux to bind to a certain PHY driver (the
> one that matches the ID given in the compatible string), ignoring the ID
> which is reported by the actual PHY.
> Whenever a device is shipped with (multiple) different PHYs during it's
> production lifetime then explicitly specifying
> "ethernet-phy-idAAAA.BBBB" could break certain revisions of that device.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
> Thanks to Andrew Lunn for pointing the documentation issue out to me in:
> http://lists.infradead.org/pipermail/linux-amlogic/2017-January/002141.html
Applied.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: Fix inverted test for multiple CPU interface
From: David Miller @ 2017-01-24 18:34 UTC (permalink / raw)
To: andrew; +Cc: vivien.didelot, f.fainelli, netdev
In-Reply-To: <1485119805-30338-1-git-send-email-andrew@lunn.ch>
From: Andrew Lunn <andrew@lunn.ch>
Date: Sun, 22 Jan 2017 22:16:45 +0100
> Remove the wrong !, otherwise we get false positives about having
> multiple CPU interfaces.
>
> Fixes: b22de490869d ("net: dsa: store CPU switch structure in the tree")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 3/3] net/tcp-fastopen: Add new API support
From: Willy Tarreau @ 2017-01-24 18:34 UTC (permalink / raw)
To: Eric Dumazet
Cc: Wei Wang, netdev, David Miller, Eric Dumazet, Yuchung Cheng,
Wei Wang
In-Reply-To: <1485279889.16328.306.camel@edumazet-glaptop3.roam.corp.google.com>
Hi Eric,
On Tue, Jan 24, 2017 at 09:44:49AM -0800, Eric Dumazet wrote:
> I believe there is a bug in this application.
>
> It does not check connect() return value.
Yes in fact it does but I noticed the same thing, there's something causing
the event not to be registered or something like this.
> When 0 is returned, it makes no sense to wait 200 ms :
>
> > 06:29:24.048593 epoll_ctl(3, EPOLL_CTL_ADD, 9, {EPOLLIN|EPOLLRDHUP, {u32=9, u64=9}}) = 0
> > 06:29:24.048651 epoll_wait(3, [], 200, 0) = 0
> > 06:29:24.048699 getsockopt(10, SOL_SOCKET, SO_ERROR, [0], [4]) = 0
>
> And it makes no sense to call connect() again :
>
> > 06:29:24.048751 connect(10, {sa_family=AF_INET, sin_port=htons(8001), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRES
> > S (Operation now in progress)
I totally agree.
> man connect
>
> <quote>
> Generally, connection-based protocol sockets may successfully connect()
> only once;
> </quote>
>
>
> I would prefer we do not add yet another bit in tcp kernel sockets, to
> work around some oddity in your program Willy.
I'm fine with chasing the bug on my side and fixing it, but there's a
semantic trouble anyway with returning -EINPROGRESS :
- connect() = 0 indicates that the connection is established
- then a further connect() should return -EISCONN, and does so when
not using TFO
man connect says this regarding EINPROGRESS :
The socket is nonblocking and the connection cannot be completed immediately.
It is possible to select(2) or poll(2) for completion by selecting the
socket for writing. After select(2) indicates writability, use getsockopt(2)
to read the SO_ERROR option at level SOL_SOCKET to determine whether connect()
completed successfully (SO_ERROR is zero) or unsuccess-fully (SO_ERROR is
one of the usual error codes listed here, explaining the reason for the failure).
Here we clearly have an incompatibility between this EINPROGRESS saying
that we must poll, and poll returning POLLOUT suggesting that it's now
OK.
I'm totally fine with not using an extra bit in a scarce area, but then
we can either add an extra argument to __inet_stream_connect() to say
"this is sendmsg" or just add an extra flag in the last argument.
But in general I don't feel comfortable with a semantics that doesn't
completely match the current and documented one :-/
Thanks,
Willy
^ permalink raw reply
* Re: [PATCH net-next v5 0/2] stmmac: dwmac-meson8b: configurable RGMII TX delay
From: David Miller @ 2017-01-24 18:36 UTC (permalink / raw)
To: martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
carlo-KA+7E9HrN00dnm+yROfE0A, khilman-rdvid1DuHRBWk0Htik3J/w,
narmstrong-rdvid1DuHRBWk0Htik3J/w
In-Reply-To: <20170122220246.13602-1-martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
From: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Date: Sun, 22 Jan 2017 23:02:44 +0100
> Currently the dwmac-meson8b stmmac glue driver uses a hardcoded 1/4
> cycle (= 2ns) TX clock delay. This seems to work fine for many boards
> (for example Odroid-C2 or Amlogic's reference boards) but there are
> some others where TX traffic is simply broken.
> There are probably multiple reasons why it's working on some boards
> while it's broken on others:
> - some of Amlogic's reference boards are using a Micrel PHY
> - hardware circuit design
> - maybe more...
>
> iperf3 results on my Mecool BB2 board (Meson GXM, RTL8211F PHY) with
> TX clock delay disabled on the MAC (as it's enabled in the PHY driver).
> TX throughput was virtually zero before:
...
> I get similar TX throughput on my Meson GXBB "MXQ Pro+" board when I
> disable the PHY's TX-delay and configure a 4ms TX-delay on the MAC.
> So changes to at least the RTL8211F PHY driver are needed to get it
> working properly in all situations.
Series applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: couple mdio_module_driver changes
From: David Miller @ 2017-01-24 18:38 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, andrew, vivien.didelot
In-Reply-To: <20170123051733.10316-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Sun, 22 Jan 2017 21:17:31 -0800
> Small patch series fixing a comment for mdio_module_driver and
> finally utilizing it in b53_mdio.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net] r8152: don't execute runtime suspend if the tx is not empty
From: David Miller @ 2017-01-24 18:39 UTC (permalink / raw)
To: hayeswang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <1394712342-15778-235-Taiwan-albertk@realtek.com>
From: Hayes Wang <hayeswang@realtek.com>
Date: Mon, 23 Jan 2017 14:18:43 +0800
> Runtime suspend shouldn't be executed if the tx queue is not empty,
> because the device is not idle.
>
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH net] net/sched: matchall: Fix configuration race
From: David Miller @ 2017-01-24 18:42 UTC (permalink / raw)
To: yotamg; +Cc: mlxsw, jiri, eladr, daniel, jhs, netdev
In-Reply-To: <1485161667-13929-1-git-send-email-yotamg@mellanox.com>
From: Yotam Gigi <yotamg@mellanox.com>
Date: Mon, 23 Jan 2017 10:54:27 +0200
> In the current version, the matchall internal state is split into two
> structs: cls_matchall_head and cls_matchall_filter. This makes little
> sense, as matchall instance supports only one filter, and there is no
> situation where one exists and the other does not. In addition, that led
> to some races when filter was deleted while packet was processed.
>
> Unify that two structs into one, thus simplifying the process of matchall
> creation and deletion. As a result, the new, delete and get callbacks have
> a dummy implementation where all the work is done in destroy and change
> callbacks, as was done in cls_cgroup.
>
> Fixes: bf3994d2ed31 ("net/sched: introduce Match-all classifier")
> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
> Acked-by: Jiri Pirko <jiri@mellanox.com>
This doesn't apply cleanly to the net tree, please respin.
^ permalink raw reply
* Re: [PATCH net-next 3/3] net/tcp-fastopen: Add new API support
From: Willy Tarreau @ 2017-01-24 18:43 UTC (permalink / raw)
To: Eric Dumazet
Cc: Yuchung Cheng, Wei Wang, netdev, David Miller, Eric Dumazet,
Wei Wang
In-Reply-To: <1485279727.16328.303.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, Jan 24, 2017 at 09:42:07AM -0800, Eric Dumazet wrote:
> On Tue, 2017-01-24 at 09:26 -0800, Yuchung Cheng wrote:
>
> > >
> > > Do you think there's a compelling reason for adding a new option or
> > > are you interested in a small patch to perform the change above ?
> > I like the proposal especially other stack also uses TCP_FASTOPEN
> > https://msdn.microsoft.com/en-us/library/windows/desktop/ms738596(v=vs.85).aspx
>
>
> Problem is that might break existing applications that were using
> TCP_FASTOPEN before a connect() (it was a NOP until now)
>
> I prefer we use a separate new option to be 100% safe, not adding
> regressions.
>
> Only new applications, tested, will use this new feature at their risk.
That's indeed a good point. I Yuchung's comment above made me wonder
about application's portability but very few OSes will use this and in
the end it might be that portable applications will just add :
#define TCP_FASTOPEN_CONNECT TCP_FASTOPEN
For other OSes and use TCP_FASTOPEN_CONNECT only for the connect() case.
Willy
^ permalink raw reply
* Re: [patch net-next v2 0/4] Add support for offloading packet-sampling
From: David Miller @ 2017-01-24 18:44 UTC (permalink / raw)
To: jiri
Cc: netdev, yotamg, idosch, eladr, nogahf, ogerlitz, jhs,
geert+renesas, stephen, xiyou.wangcong, linux, roopa,
john.fastabend, simon.horman, mrv
In-Reply-To: <1485166031-4773-1-git-send-email-jiri@resnulli.us>
From: Jiri Pirko <jiri@resnulli.us>
Date: Mon, 23 Jan 2017 11:07:07 +0100
> From: Jiri Pirko <jiri@mellanox.com>
>
> Yotam says:
>
> The first patch introduces the psample module, a netlink channel dedicated
> to packet sampling implemented using generic netlink. This module provides
> a generic way for kernel modules to sample packets, while not being tied
> to any specific subsystem like NFLOG.
>
> The second patch adds the sample tc action, which uses psample to randomly
> sample packets that match a classifier. The user can configure the psample
> group number, the sampling rate and the packet's truncation (to save
> kernel-user traffic).
>
> The last two patches add the support for offloading the matchall-sample
> tc command in the mlxsw driver, for ingress qdiscs.
>
> An example for psample usage can be found in the libpsample project at:
> https://github.com/Mellanox/libpsample
Series applied, thank you.
^ permalink raw reply
* Re: [patch net] mlxsw: spectrum_router: Correctly reallocate adjacency entries
From: David Miller @ 2017-01-24 18:45 UTC (permalink / raw)
To: jiri; +Cc: netdev, idosch, eladr
In-Reply-To: <1485166302-5629-1-git-send-email-jiri@resnulli.us>
From: Jiri Pirko <jiri@resnulli.us>
Date: Mon, 23 Jan 2017 11:11:42 +0100
> From: Ido Schimmel <idosch@mellanox.com>
>
> mlxsw_sp_nexthop_group_mac_update() is called in one of two cases:
>
> 1) When the MAC of a nexthop needs to be updated
> 2) When the size of a nexthop group has changed
>
> In the second case the adjacency entries for the nexthop group need to
> be reallocated from the adjacency table. In this case we must write to
> the entries the MAC addresses of all the nexthops that should be
> offloaded and not only those whose MAC changed. Otherwise, these entries
> would be filled with garbage data, resulting in packet loss.
>
> Fixes: a7ff87acd995 ("mlxsw: spectrum_router: Implement next-hop routing")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Applied.
^ permalink raw reply
* Re: [PATCH v2] net: broadcom: bnx2x: use new api ethtool_{get|set}_link_ksettings
From: David Miller @ 2017-01-24 18:49 UTC (permalink / raw)
To: tremyfr; +Cc: Yuval.Mintz, ariel.elior, everest-linux-l2, netdev, linux-kernel
In-Reply-To: <1485006196-6472-1-git-send-email-tremyfr@gmail.com>
From: Philippe Reynes <tremyfr@gmail.com>
Date: Sat, 21 Jan 2017 14:43:16 +0100
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
>
> As I don't have the hardware, I'd be very pleased if
> someone may test this patch.
>
> Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
> ---
> Changelog:
> v2:
> - also move to new api for bnx2x_vf
Applied.
^ permalink raw reply
* Re: [PATCH] net: intel: e1000: use new api ethtool_{get|set}_link_ksettings
From: David Miller @ 2017-01-24 18:50 UTC (permalink / raw)
To: tremyfr; +Cc: jeffrey.t.kirsher, intel-wired-lan, netdev, linux-kernel
In-Reply-To: <1485011163-7402-1-git-send-email-tremyfr@gmail.com>
From: Philippe Reynes <tremyfr@gmail.com>
Date: Sat, 21 Jan 2017 16:06:03 +0100
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
>
> As I don't have the hardware, I'd be very pleased if
> someone may test this patch.
>
> Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
I am expecting the upstream Intel NIC folks to pick this up.
^ permalink raw reply
* Re: [PATCH net-next 3/3] net/tcp-fastopen: Add new API support
From: Eric Dumazet @ 2017-01-24 18:51 UTC (permalink / raw)
To: Willy Tarreau
Cc: Wei Wang, netdev, David Miller, Eric Dumazet, Yuchung Cheng,
Wei Wang
In-Reply-To: <20170124183446.GE21921@1wt.eu>
On Tue, 2017-01-24 at 19:34 +0100, Willy Tarreau wrote:
> Hi Eric,
>
> On Tue, Jan 24, 2017 at 09:44:49AM -0800, Eric Dumazet wrote:
> > I believe there is a bug in this application.
> >
> > It does not check connect() return value.
>
> Yes in fact it does but I noticed the same thing, there's something causing
> the event not to be registered or something like this.
>
> > When 0 is returned, it makes no sense to wait 200 ms :
> >
> > > 06:29:24.048593 epoll_ctl(3, EPOLL_CTL_ADD, 9, {EPOLLIN|EPOLLRDHUP, {u32=9, u64=9}}) = 0
> > > 06:29:24.048651 epoll_wait(3, [], 200, 0) = 0
> > > 06:29:24.048699 getsockopt(10, SOL_SOCKET, SO_ERROR, [0], [4]) = 0
> >
> > And it makes no sense to call connect() again :
> >
> > > 06:29:24.048751 connect(10, {sa_family=AF_INET, sin_port=htons(8001), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRES
> > > S (Operation now in progress)
>
> I totally agree.
>
> > man connect
> >
> > <quote>
> > Generally, connection-based protocol sockets may successfully connect()
> > only once;
> > </quote>
> >
> >
> > I would prefer we do not add yet another bit in tcp kernel sockets, to
> > work around some oddity in your program Willy.
>
> I'm fine with chasing the bug on my side and fixing it, but there's a
> semantic trouble anyway with returning -EINPROGRESS :
> - connect() = 0 indicates that the connection is established
> - then a further connect() should return -EISCONN, and does so when
> not using TFO
>
> man connect says this regarding EINPROGRESS :
>
> The socket is nonblocking and the connection cannot be completed immediately.
> It is possible to select(2) or poll(2) for completion by selecting the
> socket for writing. After select(2) indicates writability, use getsockopt(2)
> to read the SO_ERROR option at level SOL_SOCKET to determine whether connect()
> completed successfully (SO_ERROR is zero) or unsuccess-fully (SO_ERROR is
> one of the usual error codes listed here, explaining the reason for the failure).
>
> Here we clearly have an incompatibility between this EINPROGRESS saying
> that we must poll, and poll returning POLLOUT suggesting that it's now
> OK.
>
> I'm totally fine with not using an extra bit in a scarce area, but then
> we can either add an extra argument to __inet_stream_connect() to say
> "this is sendmsg" or just add an extra flag in the last argument.
>
> But in general I don't feel comfortable with a semantics that doesn't
> completely match the current and documented one :-/
>
> Thanks,
> Willy
We do not return -1 / EINPROGRESS but 0
Do not call connect() twice, it is clearly not supposed to work.
Fact that it happened to work is still kept for applications not using
new features (like TCP_FASTOPEN_CONNECT), we wont break this.
I would prefer you submit _if_ needed a patch on top of Wei patch, which
was carefully tested with our ~500 packetdrill tests.
TCP_FASTOPEN_CONNECT + connect() returning 0 is already a violation of
past behavior, in the sense that no connection really happened yet.
An application exploiting this return value and consider the server is
reachable would be mistaken.
We do not support connect() + TCP_FASTOPEN_CONNECT + read(), because we
do not want to add yet another conditional test in recvmsg() fast path
for such feature, while existing sendmsg() can already be used to send a
SYN with FastOpen option.
So people are not expected to blindly add TCP_FASTOPEN_CONNECT to every
TCP socket they allocate/use. This would add too much bloat to the
kernel.
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: Andy Lutomirski @ 2017-01-24 18:54 UTC (permalink / raw)
To: Alexei Starovoitov, Tejun Heo
Cc: David Ahern, Andy Lutomirski, Network Development,
David S. Miller, Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <20170124174823.GA17813@ast-mbp.thefacebook.com>
On Tue, Jan 24, 2017 at 9:48 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Mon, Jan 23, 2017 at 08:32:02PM -0800, Andy Lutomirski wrote:
>> On Mon, Jan 23, 2017 at 8:05 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
>> > On 1/23/17 8:37 PM, Andy Lutomirski wrote:
>> >> Yes, it is a bug because cgroup+bpf causes unwitting programs to be
>> >> subject to BPF code installed by a different, potentially unrelated
>> >> process. That's a new situation. The failure can happen when a
>> >> privileged supervisor (whoever runs ip vrf) runs a clueless or
>> >> unprivileged program (the thing calling unshare()).
>> >
>> > There are many, many ways to misconfigure networking and to run programs in a context or with an input argument that causes the program to not work at all, not work as expected or stop working. This situation is no different.
>> >
>> > For example, the only aspect of BPF_PROG_TYPE_CGROUP_SOCK filters that are namespace based is the ifindex. You brought up the example of changing namespaces where the ifindex is not defined. Alexei mentioned an example where interfaces can be moved to another namespace breaking any ifindex based programs. Another example is the interface can be deleted. Deleting an interface with sockets bound to it does not impact the program in any way - no notification, no wakeup, nothing. The sockets just don't work.
>>
>> And if you use 'ip vrf' to bind to a vrf with ifindex 4 and a program
>> unshares netns and creates an interface with ifindex 4, then that
>> program will end up with its sockets magically bound to ifindex 4 and
>> will silently malfunction.
>>
>> I can think of multiple ways to address this problem. You could scope
>> the hooks to a netns (which is sort of what my patch does). You could
>> find a way to force programs in a given cgroup to only execute in a
>> single netns, although that would probably cause other breakage. You
>> could improve the BPF hook API to be netns-aware, which could plausbly
>> address issues related to unshare() but might get very tricky when
>> setns() is involved.
>
> scoping cgroup to netns will create weird combination of cgroup and netns
> which was never done before. cgroup and netns scopes have to be able
> to overlap in arbitrary way. Application shouldn't not be able to escape
> cgroup scope by changing netns.
I had assumed that too, but I'm not longer at all convinced that this
is a problem. It's certainly the case that, if you put an application
into a restrictive cgroup, it shouldn't be able to bypass those
restrictions using unshare() or setns(). But I don't think this is
really possible regardless. The easy way to try to escape is using
unshare(), but this doesn't actually buy you anything.
$ unshare -Urn ip link
1: lo: <LOOPBACK> mtu 65536 qdisc noop state DOWN mode DEFAULT group
default qlen 1000
link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
Maybe I just escaped the cgroup restrictions on ingress and egress,
but that doesn't seem to matter because I also no longer have any
interfaces that have any traffic. And, if I created a socket before
unsharing, it's still safe because that socket is bound to the old
netns and would still be subject to the cgroup rules with my patch
applied.
setns() is a bit more complicated, but it should still be fine.
netns_install() requires CAP_NET_ADMIN over the target netns, so you
can only switch in to a netns if you already have privilege in that
netns.
>> My point is that, in 4.10-rc, it doesn't work right, and I doubt this
>> problem is restricted to just 'ip vrf'. Without some kind of change
>> to the way that netns and cgroup+bpf interact, anything that uses
>> sk_bound_dev_if or reads the ifindex on an skb will be subject to a
>> huge footgun that unprivileged programs can trigger and any future
>> attempt to make the cgroup+bpf work for unprivileged users is going to
>> be more complicated than it deserves to be.
>
> For n-th time, the current BPF_PROG_TYPE_CGROUP* is root only and
> speculation about unprivileged usage are not helping the discussion.
...which has nothing to do with my example of how it's broken. I used
'ip vrf' the way it was intended to be used and then I ran code in it
that uses only APIs that predate eBPF. The result was that the
program obtained a broken socket that had sk_bound_dev_if filled in
with a nonsense index. There's no speculation -- it's just broken.
Maybe you can argue that this is a missing feature in cgroup+bpf (no
API to query which netns is in use) and a corresponding bug in 'ip
vrf', but I see this as evidence that cgroup+bpf as it exists in 4.10
is not carefully enough throught through. The only non-example user
of it that I can find (ip vrf) is buggy and can't really be fixed
using mechanisms that exist in 4.10-rc.
>
>> things up so that unshare will malfunction. It should avoid
>> malfunctioning when running Linux programs that are unaware of it.
>
> I agree that for VRF use case it will help to make programs netns
> aware by adding new bpf_get_current_netns_id() or something helper,
> but it's up to the program to function properly or be broken.
This will cause David's code to run slower, and I think he wants very
high performance.
> I will work on the patch to add that.
>
I think that may be a reasonable thing to do, but I think this may be
better as a non-default option.
Tejun, I can see two basic ways that cgroup+bpf delegation could work
down the road. Both depend on unprivileged users being able to load
BPF programs of the correct type, but that's mostly a matter of
auditing the code and doesn't have any particularly interesting design
challenges as far as I know.
Approach 1: Scope cgroup+bpf hooks to a netns and require
ns_capable(CAP_NET_ADMIN), fs permissions, and (optionally) delegation
to be enabled to install them. This shouldn't have any particularly
dangerous security implications because ns_capable(CAP_NET_ADMIN)
means you already own the netns.
Approach 2: Keep cgroup+bpf hooks global. This makes the delegation
story much trickier. There needs to be some mechanism to prevent some
program that has delegated cgroup control from affecting the behavior
of outside programs. This isn't so easy. Imagine that you've
delegated /cgroup/foo to UID 1000. A program with UID 1000 can now
install a hook on /cgroup/foo, but there needs to be a mechanism to
prevent UID 1000 from running in /cgroup/foo in the global netns and
then running a tool like sudo. This *might* be possible using just
existing cgroup mechanisms, but it's going to be quite tricky to make
sure it's done completely correctly and without dangerous races.
If cgroup+bpf stays global, then I think you're basically committing
to approach 2.
I would suggest doing approach 1 (i.e. apply my patch) and then, if
truly needed for some use case, add an option so that globally
privileged programs can create hooks that affect all namespaces.
Alexei, do you have an actual use case in mind that requires hooks to
be global? The only current uses I'm aware of seem to work better if
they're local to a netns.
^ permalink raw reply
* Re: [PATCH 0/5] simple gtp improvements
From: Harald Welte @ 2017-01-24 18:26 UTC (permalink / raw)
To: Andreas Schultz; +Cc: Pablo Neira, netdev, Lionel Gauthier, openbsc
In-Reply-To: <20170124172402.12096-1-aschultz@tpip.net>
Hi Andreas,
I agree with your changes (particularly those related to 3GPP specs)
like 2/5 and 5/5. Also, 1/5 is of course obvious.
For kernel topics like 3/5 and 4/5 I trust Pablo and the general netdev
crew to have better judgement than me.
--
- Harald Welte <laforge@netfilter.org> http://netfilter.org/
============================================================================
"Fragmentation is like classful addressing -- an interesting early
architectural error that shows how much experimentation was going
on while IP was being designed." -- Paul Vixie
^ permalink raw reply
* Re: [PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class()
From: Florian Fainelli @ 2017-01-24 18:59 UTC (permalink / raw)
To: Russell King - ARM Linux, Andrew Lunn, Greg KH
Cc: netdev, Jason Cooper, Sebastian Hesselbarth, Gregory Clement,
Vivien Didelot, David S. Miller,
moderated list:ARM SUB-ARCHITECTURES, open list
In-Reply-To: <841dc133-e9af-e7ca-75bf-8371337d6281@gmail.com>
On 01/19/2017 10:12 AM, Florian Fainelli wrote:
>
> Back to the actual code that triggered this discussion, the whole
> purpose is just a safeguard. Given a device reference, we can assume
> that it is indeed the backing device for a net_device, and we could do a
> to_net_device() right away (and crash if someone did not write correct
> platform_data structures), or, by walking the device tree (the device
> driver model one) we can make sure it does belong in the proper class
> and this is indeed what we think it is.
Greg, did Russell's explanation clarify things, or do you still think
this is completely bogus and we need to re design the whole thing?
Just asking so I can try to resubmit just the preparatory parts or just
the whole thing.
Thank you
--
Florian
^ permalink raw reply
* Re: [PATCH 5/5] gtp: let userspace handle packets for invalid tunnels
From: Pablo Neira Ayuso @ 2017-01-24 19:03 UTC (permalink / raw)
To: Andreas Schultz; +Cc: netdev, Lionel Gauthier, openbsc, Harald Welte
In-Reply-To: <20170124172402.12096-6-aschultz@tpip.net>
Hi Andreas,
On Tue, Jan 24, 2017 at 06:24:02PM +0100, Andreas Schultz wrote:
> enable userspace to send error replies for invalid tunnels
>
> Signed-off-by: Andreas Schultz <aschultz@tpip.net>
> ---
> drivers/net/gtp.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
> index 912721e..c607333 100644
> --- a/drivers/net/gtp.c
> +++ b/drivers/net/gtp.c
> @@ -198,12 +198,12 @@ static int gtp0_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb,
> pctx = gtp0_pdp_find(gtp, be64_to_cpu(gtp0->tid));
> if (!pctx) {
> netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", skb);
> - return -1;
> + return 1;
> }
>
> if (!gtp_check_src_ms(skb, pctx, hdrlen)) {
> netdev_dbg(gtp->dev, "No PDP ctx for this MS\n");
> - return -1;
> + return 1;
So userspace gets the packet that we cannot forward. I guess your
userspace codebase performs this sanity checks again so you can send
the appropriate error reply?
> }
>
> /* Get rid of the GTP + UDP headers. */
> @@ -247,12 +247,12 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb,
> pctx = gtp1_pdp_find(gtp, ntohl(gtp1->tid));
> if (!pctx) {
> netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", skb);
> - return -1;
> + return 1;
> }
>
> if (!gtp_check_src_ms(skb, pctx, hdrlen)) {
> netdev_dbg(gtp->dev, "No PDP ctx for this MS\n");
> - return -1;
> + return 1;
> }
>
> /* Get rid of the GTP + UDP headers. */
> --
> 2.10.2
>
^ permalink raw reply
* Re: [net-next PATCH v7] net: dummy: Introduce dummy virtual functions
From: David Miller @ 2017-01-24 19:07 UTC (permalink / raw)
To: phil; +Cc: netdev, sd
In-Reply-To: <20170123111733.28643-1-phil@nwl.cc>
From: Phil Sutter <phil@nwl.cc>
Date: Mon, 23 Jan 2017 12:17:33 +0100
> The idea for this was born when testing VF support in iproute2 which was
> impeded by hardware requirements. In fact, not every VF-capable hardware
> driver implements all netdev ops, so testing the interface is still hard
> to do even with a well-sorted hardware shelf.
>
> To overcome this and allow for testing the user-kernel interface, this
> patch allows to turn dummy into a PF with a configurable amount of VFs.
>
> Since my patch series 'bus-agnostic-num-vf' has been accepted,
> implementing the required interfaces is pretty straightforward: Iff
> 'num_vfs' module parameter was given a value >0, a dummy bus type is
> being registered which implements the 'num_vf()' callback. Additionally,
> a dummy parent device common to all dummy devices is registered which
> sits on the above dummy bus.
>
> Joint work with Sabrina Dubroca.
>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
Yeah this looks awesome, applied, thanks!
^ permalink raw reply
* Re: [PATCH] phy: marvell: remove conflicting initializer
From: David Miller @ 2017-01-24 19:09 UTC (permalink / raw)
To: arnd
Cc: f.fainelli, andrew, charles-antoine.couret, clemens.gruber,
netdev, linux-kernel
In-Reply-To: <20170123121905.3245589-1-arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 23 Jan 2017 13:18:41 +0100
> One line was apparently pasted incorrectly during a new feature patch:
>
> drivers/net/phy/marvell.c:2090:15: error: initialized field overwritten [-Werror=override-init]
> .features = PHY_GBIT_FEATURES,
>
> I'm removing the extraneous line here to avoid the W=1 warning and restore
> the previous flags value, and I'm slightly reordering the lines for consistency
> to make it less likely to happen again in the future. The ordering in the
> array is still not the same as in the structure definition, instead I picked
> the order that is most common in this file and that seems to make more sense
> here.
>
> Fixes: 0b04680fdae4 ("phy: marvell: Add support for temperature sensor")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied, thanks Arnd.
Please start putting appropriate "net" and "net-next" strings in your
PATCH subject lines, it will save me a lot of time.
^ permalink raw reply
* Re: [PATCH net-next 3/3] net/tcp-fastopen: Add new API support
From: Willy Tarreau @ 2017-01-24 19:11 UTC (permalink / raw)
To: Eric Dumazet
Cc: Wei Wang, netdev, David Miller, Eric Dumazet, Yuchung Cheng,
Wei Wang
In-Reply-To: <1485283885.16328.319.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, Jan 24, 2017 at 10:51:25AM -0800, Eric Dumazet wrote:
> We do not return -1 / EINPROGRESS but 0
>
> Do not call connect() twice, it is clearly not supposed to work.
Yes it is, it normally returns -1 / EISCONN on a regular socket :
EISCONN
The socket is already connected.
> Fact that it happened to work is still kept for applications not using
> new features (like TCP_FASTOPEN_CONNECT), we wont break this.
Sure but as we saw, deeply burried silent bugs having no effect in
existing applications can suddenly become problematic once TFO is
enabled, and the semantics difference between the two are minimal
enough to warrant being closed.
> I would prefer you submit _if_ needed a patch on top of Wei patch, which
> was carefully tested with our ~500 packetdrill tests.
I totally understand and rest assured that I have a great respect for
this amount of test, which is also why I find the feature really exciting.
I'll probably propose something involving an extra argument then, this
will be much easier to review in the perspective of the existing tests.
> TCP_FASTOPEN_CONNECT + connect() returning 0 is already a violation of
> past behavior, in the sense that no connection really happened yet.
I agree but semantically it could be considered that it means "connect()
already called successfully, feel free to proceed with send() whenever
you want" and that's why it's appealing ;-)
> An application exploiting this return value and consider the server is
> reachable would be mistaken.
100% agree, I even had a private discussion regarding this, mentionning
that I already added a test in haproxy to only enable it if there are
data scheduled for leaving. In my case it's easy because I already have
the same test to decide whether or not to disable TCP_QUICKACK to save
one packet by sending the payload with the first ACK. So in short it will
be :
if (data) {
if (disable_quick_ack)
setsockopt(fd, SOL_TCP, TCP_QUICKACK, &zero, sizeof(&zero));
if (enable_fastopen)
setsockopt(fd, SOL_TCP, TCP_FASTOPEN_CONNECT, &one, sizeof(&one));
}
connect(fd, ...);
But I certainly understand that in some implementations it's could be
trickier. That just reminds me that I haven't tested it combined with
splicing. I'll have to try this.
> We do not support connect() + TCP_FASTOPEN_CONNECT + read(), because we
> do not want to add yet another conditional test in recvmsg() fast path
> for such feature, while existing sendmsg() can already be used to send a
> SYN with FastOpen option.
Yes, I think the mechanism is complex enough internally not to try to
make it even more complex :-)
> So people are not expected to blindly add TCP_FASTOPEN_CONNECT to every
> TCP socket they allocate/use. This would add too much bloat to the
> kernel.
I really think that the true benefit of TFO is for HTTP and SSL where
the client speaks first and already has something to say when the decision
to connect is made. It should be clear in implementors' minds that it
cannot be a default setting and that it doesn't make sense.
Thanks,
Willy
^ permalink raw reply
* Re: [PATCH 3/5] gtp: fix cross netns recv on gtp socket
From: Pablo Neira Ayuso @ 2017-01-24 19:15 UTC (permalink / raw)
To: Andreas Schultz; +Cc: netdev, Lionel Gauthier, openbsc, Harald Welte
In-Reply-To: <20170124172402.12096-4-aschultz@tpip.net>
On Tue, Jan 24, 2017 at 06:24:00PM +0100, Andreas Schultz wrote:
> The use of the passed through netlink src_net to check for a
> cross netns operation was wrong. Using the GTP socket and the
> GTP netdevice is always correct (even if the netdev has been
> moved to new netns after link creation).
>
> Remove the now obsolete net field from gtp_dev.
The net tree can take fixes anytime, so if you target this patch to
[PATCH net] this speeds up integration into mainline kernels. Note, as
soon as this patch hits Linus tree, we can request -stable submission
so older -stable kernels can get this.
If this follows the net-next path, then this fix is going to take
several weeks (sometimes months) to show in mainline kernels.
So please add [PATCH net] or [PATCH net-next] so it's clear to
everyone what is your target, David usually requests this.
BTW, probably you can target this small fix to net, then you can
request David to pull net into net-next so the fix propagates onwards.
Sorry for this bureaucratic stuff, but given the workload we deal
with, you will really helps us if you deal with these nitpicks.
Thanks Andreas!
^ permalink raw reply
* Re: [PATCH 1/5] gtp: add genl family modules alias
From: Pablo Neira Ayuso @ 2017-01-24 19:16 UTC (permalink / raw)
To: Andreas Schultz; +Cc: netdev, Lionel Gauthier, openbsc, Harald Welte
In-Reply-To: <20170124172402.12096-2-aschultz@tpip.net>
On Tue, Jan 24, 2017 at 06:23:58PM +0100, Andreas Schultz wrote:
> Auto-load the module when userspace asks for the gtp netlink
> family.
This qualifies as fix, since autoload is broken.
You may send a batch including this for David's net tree.
^ permalink raw reply
* Re: [PATCH 2/5] gtp: clear DF bit on GTP packet tx
From: Pablo Neira Ayuso @ 2017-01-24 19:17 UTC (permalink / raw)
To: Andreas Schultz; +Cc: netdev, Lionel Gauthier, openbsc, Harald Welte
In-Reply-To: <20170124172402.12096-3-aschultz@tpip.net>
On Tue, Jan 24, 2017 at 06:23:59PM +0100, Andreas Schultz wrote:
> 3GPP TS 29.281 and 3GPP TS 29.060 imply that GTP-U packets should be
> sent with the DF bit cleared. For example 3GPP TS 29.060, Release 8,
> Section 13.2.2:
>
> > Backbone router: Any router in the backbone may fragment the GTP
> > packet if needed, according to IPv4.
Given this is fixing a broken implementation with regards to
standards, please target this to net.
^ permalink raw reply
* Re: [PATCH 4/5] gtp: remove unnecessary rcu_read_lock
From: Pablo Neira Ayuso @ 2017-01-24 19:17 UTC (permalink / raw)
To: Andreas Schultz; +Cc: netdev, Lionel Gauthier, openbsc, Harald Welte
In-Reply-To: <20170124172402.12096-5-aschultz@tpip.net>
On Tue, Jan 24, 2017 at 06:24:01PM +0100, Andreas Schultz wrote:
> The rcu read lock is hold by default in the ip input path. There
> is no need to hold it twice in the socket recv decapsulate code path.
I think this is net-next material since it is not essencial.
^ permalink raw reply
* Re: Reference counting struct inet_peer
From: David Miller @ 2017-01-24 19:20 UTC (permalink / raw)
To: dwindsor; +Cc: netdev, keescook, elena.reshetova, ishkamiel
In-Reply-To: <CAEXv5_j1cz1EUPgmAKO5OHxKcoHVLuPPVRs316CWyMpw8UtpRw@mail.gmail.com>
From: David Windsor <dwindsor@gmail.com>
Date: Mon, 23 Jan 2017 07:42:51 -0500
> struct inet_peer objects get freed when their reference count
> becomes -1, not 0 as is the usual case. Is there a reason why this
> is so?
inet peer entries that sit in the tree, but have no other reference
taken, have a reference count of zero.
Therefore, any entry which has a reference count of zero can be
safely garbage collected from the tree.
When the garbage collector purges entries with a zero refcnt, it
atomically sets the refcnt to -1 so that other threads of control
in RCU protected sections that still see this entry in the tree
will not be able to grab it for use.
The -1 marker is used as a synchronization mechanism between the
GC and lookup paths.
Once -1 is atomically set, the GC code knows that no external
reference can be created.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox