* Re: Wireless regressions in v4.15-rc1
From: Johannes Berg @ 2017-12-09 20:15 UTC (permalink / raw)
To: Kalle Valo, linux-wireless, Thorsten Leemhuis
Cc: netdev, linux-kernel, ath10k
In-Reply-To: <87po7xwajg.fsf@kamboji.qca.qualcomm.com>
On Sat, 2017-12-02 at 14:59 +0200, Kalle Valo wrote:
>
> net: netlink: Update attr validation to require exact length for some types
> https://git.kernel.org/linus/28033ae4e0f5
This was reverted, more or less, to print only a warning:
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=6e237d099fac1f73a7b6d7287bb9191f29585a4e
> Jouni fixed this already in hostapd but we also need a fix for kernel so
> that old hostapd versions continue to work:
>
> https://w1.fi/cgit/hostap/commit/?id=a2426829ce426de82d2fa47071ca41ea81c43307
>
> Jouni also found a similar problem with mesh:
>
> https://w1.fi/cgit/hostap/commit/?id=963d3149abfcbab5b83f9023bc50321f777360d1
See above for the kernel revert to let old hostapd versions continue to
work (properly only on little endian platforms) when they don't have
these fixes.
> And Johannes already submitted a revert related to wpa_supplicant:
>
> [net] Revert "net: core: maybe return -EEXIST in __dev_alloc_name" diffmboxseries
> https://patchwork.ozlabs.org/patch/843863/
That was applied.
> And with ath10k I'm now seeing this:
>
> [ 133.175508] WARNING: CPU: 2 PID: 1743 at net/mac80211/agg-tx.c:315 ___ieee80211_stop_tx_ba_session+0x1ab/0x280 [mac80211]
Just sent a fix for this, kinda a merge/patch failure.
> johannes
^ permalink raw reply
* Re: [PATCH net-next 2/2] veth: set peer GSO values
From: Solio Sarabia @ 2017-12-09 19:51 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, sthemmin, stephen, davem
In-Reply-To: <3cc96c83-846d-7441-99b1-79947ca9fcd6@gmail.com>
On Sat, Dec 09, 2017 at 09:41:25AM -0700, David Ahern wrote:
> On 12/7/17 4:40 PM, Stephen Hemminger wrote:
> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> > index f5438d0978ca..a69ad39ee57e 100644
> > --- a/drivers/net/veth.c
> > +++ b/drivers/net/veth.c
> > @@ -410,6 +410,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
> > if (ifmp && (dev->ifindex != 0))
> > peer->ifindex = ifmp->ifi_index;
> >
> > + peer->gso_max_size = dev->gso_max_size;
> > + peer->gso_max_segs = dev->gso_max_segs;
> > +
> > err = register_netdevice(peer);
> > put_net(net);
> > net = NULL;
> >
>
> What if gso changes are made after device create? They are not
> propagated to the peer device like they are on link create.
This would be a nice addition after ongoing patches are merged,
since veth usually lives in another netns. For docker, it requires
a couple of extra commands to expose the peer's netns.
^ permalink raw reply
* Re: [PATCH v5 2/2] sock: Move the socket inuse to namespace.
From: Cong Wang @ 2017-12-09 19:42 UTC (permalink / raw)
To: Tonghao Zhang
Cc: Eric Dumazet, David Miller, Eric Dumazet, Willem de Bruijn,
Linux Kernel Network Developers
In-Reply-To: <CAMDZJNX0pUv6a-=xq5qFu9vc4tmmSwgVu6YMyz3izpm-HJ_Nig@mail.gmail.com>
On Fri, Dec 8, 2017 at 9:27 PM, Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> On Sat, Dec 9, 2017 at 6:09 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Thu, Dec 7, 2017 at 9:28 PM, Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>>>
>>> Release the netlink sock created in kernel(not hold the _net_ namespace):
>>>
>>
>> You can avoid counting kernel sock by testing 'kern' in sk_alloc()
>> and testing 'sk->sk_net_refcnt' in __sk_free().
> Hi cong, if we do it in this way, we will not counter the sock created
> in kernel, right ?
Yes, it is not very useful for user-space to know how many kernel
sockets we create, IMHO, so not counting kernel sockets seems
fine.
^ permalink raw reply
* Re: kernel BUG at net/core/skbuff.c:LINE! (2)
From: Cong Wang @ 2017-12-09 19:36 UTC (permalink / raw)
To: Xin Long
Cc: syzbot, davem, kuznet, LKML, linux-sctp, network dev, Neil Horman,
syzkaller-bugs, Vlad Yasevich, yoshfuji
In-Reply-To: <CADvbK_c5Z7rE-AABGOFKXYA5v7_XLg=V1HOGzBO1X-O13aJ8Gg@mail.gmail.com>
On Fri, Dec 8, 2017 at 12:45 AM, Xin Long <lucien.xin@gmail.com> wrote:
> This isn't a sctp problem, but mld's, seems when lo's mtu became 0,
> it allocs a skb without enough space in add_grec():
Shouldn't we just set its min_mtu to ETH_MIN_MTU?
^ permalink raw reply
* Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations
From: Roman Gushchin @ 2017-12-09 19:19 UTC (permalink / raw)
To: Quentin Monnet
Cc: netdev, linux-kernel, kernel-team, ast, daniel, jakub.kicinski,
kafai, David Ahern
In-Reply-To: <0eed580a-804b-329e-7bfc-1dc5c09a1deb@netronome.com>
On Fri, Dec 08, 2017 at 03:39:43PM +0000, Quentin Monnet wrote:
> 2017-12-08 14:12 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> > On Fri, Dec 08, 2017 at 10:34:16AM +0000, Quentin Monnet wrote:
> >> 2017-12-07 18:39 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> >>> This patch adds basic cgroup bpf operations to bpftool:
> >>> cgroup list, attach and detach commands.
> >>>
> >>> Usage is described in the corresponding man pages,
> >>> and examples are provided.
> > [...]
> >>> +MAP COMMANDS
> >>> +=============
> >>> +
> >>> +| **bpftool** **cgroup list** *CGROUP*
> >>> +| **bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> >>> +| **bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
> >>> +| **bpftool** **cgroup help**
> >>> +|
> >>> +| *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
> >>
> >> Could you please give the different possible values for ATTACH_TYPE and
> >> ATTACH_FLAGS, and provide some documentation for the flags?
> >
> > I intentionally didn't include the list of possible values, as it depends
> > on the exact kernel version, and other bpftool docs are carefully avoiding
> > specifying such things.
>
> Do they? As far as I can tell the only other bpftool command that uses
> flags is the `bpftool map update`, and it does specify the possible
> values for UPDATE_FLAGS (and document them) in the man page.
You are right about UPDATE_FLAGS, but at the same time we do
not describe bpf program attributes in prog show:
**bpftool prog show** [*PROG*]
Show information about loaded programs. If *PROG* is
specified show information only about given program, otherwise
list all programs currently loaded on the system.
Output will start with program ID followed by program type and
zero or more named attributes (depending on kernel version).
I think, that actually ATTACH_TYPE is similar to PROGRAM_TYPE because
it will likely be extended in the following kernel versions.
So we should probably support specifying it in a numeric form too.
ATTACH_FLAGS are probably less volatile and will unlikely be extended often,
so we can describe them in docs and add a note about the kernel version
next time when a new flag will be added.
Anyway, I don't see any big problem in documenting current ATTACH_FLAG
and ATTACH_TYPE sets, if we think that it's a good way forward.
Thanks!
^ permalink raw reply
* [PATCH net-next] veth: fix setting peer gso values
From: Solio Sarabia @ 2017-12-09 19:11 UTC (permalink / raw)
To: davem, stephen; +Cc: netdev, sthemmin, shiny.sebastian, solio.sarabia
GSO values are not correctly set for peer and dev. When creating
the veth link, GSO attributes are set for peer device, propagate
these values from peer to dev.
Signed-off-by: Solio Sarabia <solio.sarabia@intel.com>
---
drivers/net/veth.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index a69ad39..d936c8a 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -410,8 +410,8 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
if (ifmp && (dev->ifindex != 0))
peer->ifindex = ifmp->ifi_index;
- peer->gso_max_size = dev->gso_max_size;
- peer->gso_max_segs = dev->gso_max_segs;
+ dev->gso_max_size = peer->gso_max_size;
+ dev->gso_max_segs = peer->gso_max_segs;
err = register_netdevice(peer);
put_net(net);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] net: dsa: allow XAUI phy interface mode
From: Andrew Lunn @ 2017-12-09 19:08 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Vivien Didelot, Florian Fainelli, netdev
In-Reply-To: <20171208164202.GG10595@n2100.armlinux.org.uk>
> You do seem to be correct that this only applies (in mainline) to the
> ZII rev C board, so I guess including a patch to update its dts for
> the inter-switch connection would at least be sensible.
Hi Russell
So lets have the backward compatible for a couple of kernel cycles,
and modify ZII rev now to use the correct mode.
Andrew
^ permalink raw reply
* Re: [PATCH RFC 0/4] Fixes for Marvell MII paged register access races
From: Andrew Lunn @ 2017-12-09 19:06 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Russell King - ARM Linux, netdev
In-Reply-To: <204d19c0-05eb-3b03-275e-2a6d111cd1b0@gmail.com>
> > Another potential question is whether using the mdiobus lock (which
> > excludes all other MII bus access) is best - while it has the advantage
> > of also ensuring atomicity with userspace accesses, it means that no one
> > else can access an independent PHY on the same bus while a paged access
> > is on-going. It feels like a big hammer, but I'm not convinced that we
> > will see a lot of contention on it.
>
> Regarding that last topic, this could become a fairly contended lock on
> a switch with lots (e.g: > 5-6) of built-in PHYs, all being polled
> (which is usually the case right now). One would expect that the polling
> should be limited to 2 BMSR reads to minimize the bus utilization.
Hi Florian
In this case, we probably are not doing pages reads, just normal
reads. So there should not be any more contention than there already
is.
Andrew
^ permalink raw reply
* Re: [PATCH net-next v3 2/5] net: Disable GRO_HW when generic XDP is installed on a device.
From: Alexander Duyck @ 2017-12-09 18:56 UTC (permalink / raw)
To: Michael Chan
Cc: David Miller, Netdev, Andrew Gospodarek, Ariel Elior,
everest-linux-l2
In-Reply-To: <1512800879-17934-3-git-send-email-michael.chan@broadcom.com>
On Fri, Dec 8, 2017 at 10:27 PM, Michael Chan <michael.chan@broadcom.com> wrote:
> Hardware should not aggregate any packets when generic XDP is installed.
>
> Cc: Ariel Elior <Ariel.Elior@cavium.com>
> Cc: everest-linux-l2@cavium.com
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
> net/core/dev.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 6ebd0e7..ec08ace 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1542,6 +1542,29 @@ void dev_disable_lro(struct net_device *dev)
> }
> EXPORT_SYMBOL(dev_disable_lro);
>
> +/**
> + * dev_disable_gro_hw - disable HW Generic Receive Offload on a device
> + * @dev: device
> + *
> + * Disable HW Generic Receive Offload (GRO_HW) on a net device. Must be
> + * called under RTNL. This is needed if Generic XDP is installed on
> + * the device.
> + */
> +static void dev_disable_gro_hw(struct net_device *dev)
> +{
> + struct net_device *lower_dev;
> + struct list_head *iter;
> +
> + dev->wanted_features &= ~NETIF_F_GRO_HW;
> + netdev_update_features(dev);
> +
> + if (unlikely(dev->features & NETIF_F_GRO_HW))
> + netdev_WARN(dev, "failed to disable GRO_HW!\n");
> +
> + netdev_for_each_lower_dev(dev, lower_dev, iter)
> + dev_disable_gro_hw(lower_dev);
I think these two lines are redundant in dev_disable_lro, since
netdev_update_features should propagate the disable to all of the
lower devices. Also this doesn't prevent the lower devices from
re-enabling gro_hw.
> +}
> +
> static int call_netdevice_notifier(struct notifier_block *nb, unsigned long val,
> struct net_device *dev)
> {
> @@ -4564,6 +4587,7 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp)
> } else if (new && !old) {
> static_key_slow_inc(&generic_xdp_needed);
> dev_disable_lro(dev);
> + dev_disable_gro_hw(dev);
> }
> break;
>
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH net-next v3 1/5] net: Introduce NETIF_F_GRO_HW.
From: Alexander Duyck @ 2017-12-09 18:50 UTC (permalink / raw)
To: Michael Chan
Cc: David Miller, Netdev, Andrew Gospodarek, Ariel Elior,
everest-linux-l2
In-Reply-To: <1512800879-17934-2-git-send-email-michael.chan@broadcom.com>
On Fri, Dec 8, 2017 at 10:27 PM, Michael Chan <michael.chan@broadcom.com> wrote:
> Introduce NETIF_F_GRO_HW feature flag for NICs that support hardware
> GRO. With this flag, we can now independently turn on or off hardware
> GRO when GRO is on. Previously, drivers were using NETIF_F_GRO to
> control hardware GRO and so it cannot be independently turned on or
> off without affecting GRO.
>
> Hardware GRO (just like GRO) guarantees that packets can be re-segmented
> by TSO/GSO to reconstruct the original packet stream. It is a subset of
> NETIF_F_GRO and depends on it, as well as NETIF_F_RXCSUM.
So I would disagree with it being a subset of NETIF_F_GRO. If anything
it is an alternative to NETIF_F_GRO. It is performing GRO much earlier
at the device level in the case of hardware drivers. My concern is
this is probably going to end up applying to things other than just
hardware drivers though. For example what is to prevent this from
being applied to something like a virtio/tap interface? It seems like
this should be something that would be easy to implement in software.
In addition as I said in my earlier comments I think we should
probably look at using this new feature bit to indicate that we allow
GRO to occur at or below this device as opposed to just above it as
currently occurs with conventional GRO.
> Since NETIF_F_GRO is not propagated between upper and lower devices,
> NETIF_F_GRO_HW should follow suit since it is a subset of GRO. In other
> words, a lower device can independent have GRO/GRO_HW enabled or disabled
> and no feature propagation is required. This will preserve the current
> GRO behavior.
I'm going to back off on my requirement for you to handle propagation
since after spending a couple hours working on it I did find it was
more complex then I originally thought it would be. With that said
however I would want to see this feature implemented in such a way
that we can deal with propagating the bits in the future if we need to
and that is what I am basing my comments on. My concern is when this
ends up breaking we need to have a way to fix it and I don't want that
fix to end up being having to disable GRO across the board.
> Cc: Ariel Elior <Ariel.Elior@cavium.com>
> Cc: everest-linux-l2@cavium.com
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
> Documentation/networking/netdev-features.txt | 8 ++++++++
> include/linux/netdev_features.h | 3 +++
> net/core/dev.c | 12 ++++++++++++
> net/core/ethtool.c | 1 +
> 4 files changed, 24 insertions(+)
>
> diff --git a/Documentation/networking/netdev-features.txt b/Documentation/networking/netdev-features.txt
> index 7413eb0..8f36527 100644
> --- a/Documentation/networking/netdev-features.txt
> +++ b/Documentation/networking/netdev-features.txt
> @@ -163,3 +163,11 @@ This requests that the NIC receive all possible frames, including errored
> frames (such as bad FCS, etc). This can be helpful when sniffing a link with
> bad packets on it. Some NICs may receive more packets if also put into normal
> PROMISC mode.
> +
> +* rx-gro-hw
> +
> +This requests that the NIC enables Hardware GRO (generic receive offload).
> +Hardware GRO is basically the exact reverse of TSO, and is generally
> +stricter than Hardware LRO. A packet stream merged by Hardware GRO must
> +be re-segmentable by GSO or TSO back to the exact original packet stream.
> +Hardware GRO is dependent on GRO and RXCSUM.
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index b1b0ca7..db84c51 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -78,6 +78,8 @@ enum {
> NETIF_F_HW_ESP_TX_CSUM_BIT, /* ESP with TX checksum offload */
> NETIF_F_RX_UDP_TUNNEL_PORT_BIT, /* Offload of RX port for UDP tunnels */
>
> + NETIF_F_GRO_HW_BIT, /* Hardware Generic receive offload */
> +
> /*
> * Add your fresh new feature above and remember to update
> * netdev_features_strings[] in net/core/ethtool.c and maybe
> @@ -97,6 +99,7 @@ enum {
> #define NETIF_F_FRAGLIST __NETIF_F(FRAGLIST)
> #define NETIF_F_FSO __NETIF_F(FSO)
> #define NETIF_F_GRO __NETIF_F(GRO)
> +#define NETIF_F_GRO_HW __NETIF_F(GRO_HW)
> #define NETIF_F_GSO __NETIF_F(GSO)
> #define NETIF_F_GSO_ROBUST __NETIF_F(GSO_ROBUST)
> #define NETIF_F_HIGHDMA __NETIF_F(HIGHDMA)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index e32cf5c..6ebd0e7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7424,6 +7424,18 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
> features &= ~dev->gso_partial_features;
> }
>
> + if (features & NETIF_F_GRO_HW) {
> + /* Hardware GRO depends on GRO and RXCSUM. */
> + if (!(features & NETIF_F_GRO)) {
> + netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no GRO feature.\n");
> + features &= ~NETIF_F_GRO_HW;
> + }
I still disagree with this bit. I think GRO is a pure software
offload, whereas GRO_HW can represent either a software offload of
some sort occurring in or before the driver, or in the hardware.
Basically the difference between the two as I view it is where the GRO
is occurring. I would like to keep that distinction and make use of
it. As I mentioned before in the case of bonding we currently have no
way to disable GRO on the lower devices partially because GRO is a
pure software feature and always happens at each device along the way.
The nice thing about this new bit is the assumption is that it is
pushing GRO to the lowest possible level and not triggering any side
effects like GRO currently does. I hope to use that logic with stacked
devices so that we could clear the bit and have it disable GRO,
GRO_HW, and LRO on all devices below the device that cleared it.
I think this linking of GRO and GRO_HW is something that would be
better served by moving it into the driver if you are wanting to
maintain the behavior of how this was previously linked to GRO. It
also makes it so that it is much easier to compare the performance for
GRO_HW against just a pure software GRO since you could then enable
them independently. Software GRO can come at a cost, and leaving it
enabled when you want to do it all in hardware is just adding a
penalty of sorts since I know for many of my routing tests I normally
disable GRO as it has a significant per-packet cost for small packet
workloads.
> + if (!(features & NETIF_F_RXCSUM)) {
> + netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no RXCSUM feature.\n");
> + features &= ~NETIF_F_GRO_HW;
> + }
So I was thinking about this. For LRO it makes sense to disable it in
the case of RXCSUM being disabled since most implementations leave the
Rx checksum mangled. However for GRO I am not sure it makes complete
sense. For software GRO we perform checksum validation in either
tcp4_gro_receive or tcp6_gro_receive. Why should the hardware
implementation behave differently? When a GRO frame is assembled the
checksum is converted to CHECKSUM_PARTIAL anyway even if Rx checksum
validation is disabled for the driver.
I think this may be a hardware/driver specific implementation detail
and may not be generic enough to belong here. Regular GRO works
without RXCSUM, so why should we make an exception for the hardware
based version? I know in the case of the Intel NICs we don't ever
actually disable the checksum validation, we just don't report the
result we were given from the hardware and hand all the frames up the
stack. If we were implementing something like this we could still
support GRO in the hardware without reporting Rx check-sums otherwise.
The alternative way to look at it is that we shouldn't support any
form of packet mangling at he driver level if RXCSUM is disabled. In
which case, I would say we should probably frame it that way and
disable both LRO and GRO_HW if RXCSUM is enabled because this is
another case where this looks more like LRO than GRO.
> + }
> +
> return features;
> }
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index f8fcf45..50a7920 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -73,6 +73,7 @@ int ethtool_op_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
> [NETIF_F_LLTX_BIT] = "tx-lockless",
> [NETIF_F_NETNS_LOCAL_BIT] = "netns-local",
> [NETIF_F_GRO_BIT] = "rx-gro",
> + [NETIF_F_GRO_HW_BIT] = "rx-gro-hw",
> [NETIF_F_LRO_BIT] = "rx-lro",
>
> [NETIF_F_TSO_BIT] = "tx-tcp-segmentation",
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: Waiting for the PHY to complete auto-negotiation
From: Florian Fainelli @ 2017-12-09 18:49 UTC (permalink / raw)
To: Mason, Andrew Lunn; +Cc: netdev, David Miller
In-Reply-To: <89c47196-fb02-267c-409f-db48a3d07b68@free.fr>
On 12/07/2017 08:17 AM, Mason wrote:
> On 07/12/2017 00:00, Florian Fainelli wrote:
>
>> On 12/06/2017 11:25 AM, Mason wrote:
>>
>>> When we detect link down, we put the ethernet HW block in reset,
>>> and repeat initialization when the link comes back up.
>>>
>>> Hmmm, however, at the moment, I only reset on an administrative
>>> (user-requested) link down, i.e. through ndo_stop. I would probably
>>> have to handle cable unplug/replug events as well.
>>>
>>> Or just consider the quirk to make flow control too complicated
>>> to implement correctly...
>>
>> I suppose your procedure is fine, but don't you have a better way to
>> resolve that by trying to place a special RX DMA ring entry that allows
>> your RX DMA not to be entirely stopped, but intentionally looped through
>> a buffer that you control? As long as you can stop the Ethernet MAC RX,
>> working with such a limitation is probably fine, but this really sounds
>> like a huge pain in the butt and a major HW flaw.
>
> Could you elaborate a bit on your suggestion?
> (Special ring entry, looped through a buffer under my control)
> Is this a typical thing to do to stop DMA?
What I was thinking is something along the lines of what step 5 does
already, without requiring the need to loop back and sending 5 packets,
just placing an EOC descriptor, and forcing a reduction of the RX ring
size such that you hit that descriptor right away, and have DMA be
contained within that single entry in order to safely stop it. Question
is of course, how to take it out of that state considering that usually
the RX DMA produces status bits (unlike TX where the CPU produces those
bits).
>
> Currently the driver tries to stop DMA in nb8800_dma_stop(),
> which does the following:
>
> http://elixir.free-electrons.com/linux/latest/source/drivers/net/ethernet/aurora/nb8800.c#L881
>
> 1) poll until TX finishes (I assume the system no longer accepts new
> frames to send at this point)
> 2) set the EOC (end of chain) bit on all descriptors (could there be
> a problem if we receive a frame at that moment? Don't we need some
> kind of lock?)
If you have disabled the RX interrupt and quiesced NAPI, this should be
safe.
> 3) disable address filtering (need to check what this does)
> 4) enable loop-back mode
> 5) send up to 5 "fake" packets in order to hit an EOC descriptor
That's quite unusual.
>
> The reason I'm trying to move away from this method is that it doesn't
> work on our new SoC; and when pressed, the HW dev said it had never been
> supported. (Also I find it somewhat hackish, but that's a matter of taste.)
Having any HW state machine requiring X number of clock cycles to
guarantee a full transition to a stopped state is not unusual, however,
the fact that you need to send 5 packets to guarantee an EOC descriptor
is hit is completely unusual. Ideally there is a single bit that tells
the DMA engine: you are enabled, do your thing, or you are now disabled,
and you must stop all accesses to DRAM *now*.
So what would be the correct way to quiesce that controller according to
your HW folks?
--
Florian
^ permalink raw reply
* Re: RFC(v2): Audit Kernel Container IDs
From: Casey Schaufler @ 2017-12-09 18:28 UTC (permalink / raw)
To: Mickaël Salaün, Richard Guy Briggs,
cgroups-u79uwXL29TY76Z2rM5mHXA, Linux Containers, Linux API,
Linux Audit, Linux FS Devel, Linux Kernel,
Linux Network Development
Cc: mszeredi-H+wXaHxf7aLQT0dZR+AlfA, Eric W. Biederman, Simo Sorce,
jlayton-H+wXaHxf7aLQT0dZR+AlfA, Carlos O'Donell,
David Howells, Al Viro, Andy Lutomirski, Eric Paris,
trondmy-7I+n7zu2hftEKMMhf/gKZA, Michael Kerrisk
In-Reply-To: <7ebca85a-425c-2b95-9a5f-59d81707339e-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
On 12/9/2017 2:20 AM, Micka�l Sala�n wrote:
> On 12/10/2017 18:33, Casey Schaufler wrote:
>> On 10/12/2017 7:14 AM, Richard Guy Briggs wrote:
>>> Containers are a userspace concept. The kernel knows nothing of them.
>>>
>>> The Linux audit system needs a way to be able to track the container
>>> provenance of events and actions. Audit needs the kernel's help to do
>>> this.
>>>
>>> Since the concept of a container is entirely a userspace concept, a
>>> registration from the userspace container orchestration system initiates
>>> this. This will define a point in time and a set of resources
>>> associated with a particular container with an audit container ID.
>>>
>>> The registration is a pseudo filesystem (proc, since PID tree already
>>> exists) write of a u8[16] UUID representing the container ID to a file
>>> representing a process that will become the first process in a new
>>> container. This write might place restrictions on mount namespaces
>>> required to define a container, or at least careful checking of
>>> namespaces in the kernel to verify permissions of the orchestrator so it
>>> can't change its own container ID. A bind mount of nsfs may be
>>> necessary in the container orchestrator's mntNS.
>>> Note: Use a 128-bit scalar rather than a string to make compares faster
>>> and simpler.
>>>
>>> Require a new CAP_CONTAINER_ADMIN to be able to carry out the
>>> registration.
>> Hang on. If containers are a user space concept, how can
>> you want CAP_CONTAINER_ANYTHING? If there's not such thing as
>> a container, how can you be asking for a capability to manage
>> them?
>>
>>> At that time, record the target container's user-supplied
>>> container identifier along with the target container's first process
>>> (which may become the target container's "init" process) process ID
>>> (referenced from the initial PID namespace), all namespace IDs (in the
>>> form of a nsfs device number and inode number tuple) in a new auxilliary
>>> record AUDIT_CONTAINER with a qualifying op=$action field.
> Here is an idea to avoid privilege problems or the need for a new
> capability: make it automatic. What makes a container a container seems
> to be the use of at least a namespace.
You might think so, but I am assured that you can have a container
without using namespaces. Intel's "Clear Containers", which use
virtualization technology, are one example. I have considered creating
"Smack Containers" using mandatory access control technology, more
to press the point that "containers" is a marketing concept, not
technology.
> What about automatically create
> and assign an ID to a process when it enters a namespace different than
> one of its parent process? This delegates the (permission)
> responsibility to the use of namespaces (e.g. /proc/sys/user/max_* limit).
That gets ugly when you have a container that uses user, filesystem,
network and whatever else namespaces. If all containers used the same
set of namespaces I think this would be a fine idea, but they don't.
> One interesting side effect of this approach would be to be able to
> identify which processes are in the same set of namespaces, even if not
> spawn from the container but entered after its creation (i.e. using
> setns), by creating container IDs as a (deterministic) checksum from the
> /proc/self/ns/* IDs.
>
> Since the concern is to identify a container, I think the ability to
> audit the switch from one container ID to another is enough. I don't
> think we need nested IDs.
Because a container doesn't have to use namespaces to be a container
you still need a mechanism for a process to declare that it is in fact
in a container, and to identify the container.
>
> As a side note, you may want to take a look at the Linux-VServer's XID.
>
> Regards,
> Micka�l
>
^ permalink raw reply
* Re: [PATCH RFC 0/4] Fixes for Marvell MII paged register access races
From: Florian Fainelli @ 2017-12-09 18:22 UTC (permalink / raw)
To: Russell King - ARM Linux, Andrew Lunn; +Cc: netdev
In-Reply-To: <20171208164446.GH10595@n2100.armlinux.org.uk>
On 12/08/2017 08:44 AM, Russell King - ARM Linux wrote:
> On Fri, Dec 08, 2017 at 05:17:14PM +0100, Andrew Lunn wrote:
>> Hi Russell
>>
>>> There is an open question whether there should be generic helpers for
>>> this. Generic helpers would mean:
>>>
>>> - Additional couple of function pointers in phy_driver to read/write the
>>> paging register. This has the restriction that there must only be one
>>> paging register.
>>
>> I must be missing something. I don't see why there is this
>> restriction. Don't we just need
>>
>> int phy_get_page(phydev);
>> int phy_set_page(phydev, page);
>
> The restriction occurs because a PHY may have several different
> registers, and knowing which of the registers need touching becomes an
> issue. We wouldn't want these accessors to needlessly access several
> registers each and every time we requested an access to the page
> register.
>
> There's also the issue of whether an "int" or whatever type we choose to
> pass the "page" around is enough bits. I haven't surveyed all the PHY
> drivers yet to know the answer to that.
I have not come across a PHY yet that required writing a page across two
16-bit quantities, in general, the page fits within less than 16-bit
actually to fit within one MDIO write. That does not mean it cannot
exist obviously, but having about 32-bit x pages of address space within
a PHY sounds a bit extreme.
--
Florian
^ permalink raw reply
* Re: [PATCH RFC 3/4] net: phy: add unlocked accessors
From: Florian Fainelli @ 2017-12-09 18:22 UTC (permalink / raw)
To: Russell King, Andrew Lunn; +Cc: netdev
In-Reply-To: <E1eNKtR-0007nR-AH@rmk-PC.armlinux.org.uk>
On 12/08/2017 07:48 AM, Russell King wrote:
> Add unlocked versions of the bus accessors, which allows access to the
> bus with all the tracing. These accessors validate that the bus mutex
> is held, which is a basic requirement for all mii bus accesses.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> include/linux/phy.h | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 71d777fe6c3d..964803bd7324 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -716,6 +716,18 @@ static inline int phy_read(struct phy_device *phydev, u32 regnum)
> }
>
> /**
> + * __phy_read - convenience function for reading a given PHY register
> + * @phydev: the phy_device struct
> + * @regnum: register number to read
> + *
> + * The caller must have taken the MDIO bus lock.
> + */
> +static inline int __phy_read(struct phy_device *phydev, u32 regnum)
Do you know if we could have sparse validate that the caller of these
functions holds the mutex? I remember reading somewhere that sparse does
not do that yet, but can't get my hands on it.
--
Florian
^ permalink raw reply
* Re: [PATCH RFC 2/4] net: phy: use unlocked accessors for indirect MMD accesses
From: Florian Fainelli @ 2017-12-09 18:21 UTC (permalink / raw)
To: Russell King, Andrew Lunn; +Cc: netdev
In-Reply-To: <E1eNKtM-0007nK-5e@rmk-PC.armlinux.org.uk>
On 12/08/2017 07:48 AM, Russell King wrote:
> Use unlocked accessors for indirect MMD accesses to clause 22 PHYs.
> This permits tracing of these accesses.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH RFC 1/4] net: mdiobus: add unlocked accessors
From: Florian Fainelli @ 2017-12-09 18:20 UTC (permalink / raw)
To: Russell King, Andrew Lunn; +Cc: netdev
In-Reply-To: <E1eNKtH-0007nA-1a@rmk-PC.armlinux.org.uk>
On 12/08/2017 07:48 AM, Russell King wrote:
> Add unlocked versions of the bus accessors, which allows access to the
> bus with all the tracing. These accessors validate that the bus mutex
> is held, which is a basic requirement for all mii bus accesses.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH RFC 0/4] Fixes for Marvell MII paged register access races
From: Florian Fainelli @ 2017-12-09 18:19 UTC (permalink / raw)
To: Russell King - ARM Linux, Andrew Lunn; +Cc: netdev
In-Reply-To: <20171208154756.GF10595@n2100.armlinux.org.uk>
On 12/08/2017 07:47 AM, Russell King - ARM Linux wrote:
> Hi,
>
> While doing final testing of the mvneta changes for phylink, a very
> easy to trigger race condition was found with the Marvell PHY driver
> which manifested itself as the link going down when a hibernate cycle
> terminates.
>
> The issue turned out to be a race between two threads accessing the
> PHY - one trying to do a status read and the other configuring the PHY.
>
> The result is the configuration thread tries to read-modify-write a
> paged register in a non-copper page, but the status read thread
> switches the PHY back to the copper page half-way through.
>
> Various solutions involving phy->lock were considered, but found to
> create more lock dependency issues than were nice to deal with.
I can certainly imagine that, because you would ideally want a
phy_device wide lock which serializes the page entry/exit, which needs
to be able to run within the PHY state machine (so with phydev->lock
held), but also from a context where phydev->lock is not held, wheee.
>
> The solution proposed here uses the mdiobus lock to ensure that accesses
> to paged registers become atomic with respect to all other bus accesses,
> including those from userspace.
>
> There is an open question whether there should be generic helpers for
> this. Generic helpers would mean:
>
> - Additional couple of function pointers in phy_driver to read/write the
> paging register. This has the restriction that there must only be one
> paging register.
>
> - The helpers become more expensive, and because they're in a separate
> compilation unit, the compiler will be unable to optimise them by
> inlining the static functions.
>
> - The helpers would be re-usable, saving replications of that code, and
> making it more likely for phy authors to safely access the PHY.
>
> Another potential question is whether using the mdiobus lock (which
> excludes all other MII bus access) is best - while it has the advantage
> of also ensuring atomicity with userspace accesses, it means that no one
> else can access an independent PHY on the same bus while a paged access
> is on-going. It feels like a big hammer, but I'm not convinced that we
> will see a lot of contention on it.
Regarding that last topic, this could become a fairly contended lock on
a switch with lots (e.g: > 5-6) of built-in PHYs, all being polled
(which is usually the case right now). One would expect that the polling
should be limited to 2 BMSR reads to minimize the bus utilization.
>
> Comments?
>
> drivers/net/phy/marvell.c | 365 +++++++++++++++++++++------------------------
> drivers/net/phy/mdio_bus.c | 65 ++++++--
> drivers/net/phy/phy-core.c | 11 +-
> include/linux/mdio.h | 3 +
> include/linux/phy.h | 26 ++++
> 5 files changed, 256 insertions(+), 214 deletions(-)
>
--
Florian
^ permalink raw reply
* Re: [PATCH net-next] libbpf: add function to setup XDP
From: Alexei Starovoitov @ 2017-12-09 17:49 UTC (permalink / raw)
To: David Ahern; +Cc: Eric Leblond, netdev, linux-kernel, ast, daniel
In-Reply-To: <446f0215-542c-d482-109b-20149a7ff28f@gmail.com>
On Sat, Dec 09, 2017 at 09:34:46AM -0700, David Ahern wrote:
> On 12/9/17 7:43 AM, Eric Leblond wrote:
> > + /* started nested attribute for XDP */
> > + nla = (struct nlattr *)(((char *)&req)
> > + + NLMSG_ALIGN(req.nh.nlmsg_len));
> > + nla->nla_type = NLA_F_NESTED | 43/*IFLA_XDP*/;
>
> as a part of the move into libbpf can the magic numbers be replaced by
> the names directly and there as a comment?
In general it would be nice to use names instead of numbers,
but it's much bigger change then this patch, since it would require
copying and syncing a bunch of headers into tools/ which may not be such
a good idea in the end.
Only removal of min() looks a bit suspicious to me.
Eric, is it because it now comes from some header?
^ permalink raw reply
* Re: [PATCH] slip: sl_alloc(): remove unused parameter "dev_t line"
From: Oliver Hartkopp @ 2017-12-09 17:28 UTC (permalink / raw)
To: Marc Kleine-Budde, netdev; +Cc: kernel, linux-can
In-Reply-To: <39d83463-b20d-3739-7e84-f1f51919e9e5@pengutronix.de>
On 12/08/2017 12:22 PM, Marc Kleine-Budde wrote:
> Hello Oliver,
>
> I've the corresponding slcan patch already in my queue.
Excellent :-)
Thanks,
Oliver
>
> Marc
>
> On 12/08/2017 12:18 PM, Marc Kleine-Budde wrote:
>> The first and only parameter of sl_alloc() is unused, so remove it.
>>
>> Fixes: 5342b77c4123 slip: ("Clean up create and destroy")
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>> drivers/net/slip/slip.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
>> index cc63102ca96e..8940417c30e5 100644
>> --- a/drivers/net/slip/slip.c
>> +++ b/drivers/net/slip/slip.c
>> @@ -731,7 +731,7 @@ static void sl_sync(void)
>>
>>
>> /* Find a free SLIP channel, and link in this `tty' line. */
>> -static struct slip *sl_alloc(dev_t line)
>> +static struct slip *sl_alloc(void)
>> {
>> int i;
>> char name[IFNAMSIZ];
>> @@ -809,7 +809,7 @@ static int slip_open(struct tty_struct *tty)
>>
>> /* OK. Find a free SLIP channel to use. */
>> err = -ENFILE;
>> - sl = sl_alloc(tty_devnum(tty));
>> + sl = sl_alloc();
>> if (sl == NULL)
>> goto err_exit;
>>
>>
>
>
^ permalink raw reply
* Re: kernel BUG at net/core/skbuff.c:LINE! (2)
From: Eric Dumazet @ 2017-12-09 16:59 UTC (permalink / raw)
To: Xin Long, syzbot
Cc: davem, kuznet, LKML, linux-sctp, network dev, Neil Horman,
syzkaller-bugs, Vlad Yasevich, yoshfuji
In-Reply-To: <CADvbK_cnxEgU--FO+ATGL+fe2z59XVzRGzKxp1-vZxL8x8LmKA@mail.gmail.com>
On Sat, 2017-12-09 at 19:23 +0800, Xin Long wrote:
> On Fri, Dec 8, 2017 at 4:45 PM, Xin Long <lucien.xin@gmail.com>
> wrote:
> > On Fri, Dec 8, 2017 at 4:16 PM, syzbot
> > <bot+ed0838d0fa4c4f2b528e20286e6dc63effc7c14d@syzkaller.appspotmail
> > .com>
> > wrote:
> > > syzkaller has found reproducer for the following crash on
> > > 82bcf1def3b5f1251177ad47c44f7e17af039b4b
> > > git://git.cmpxchg.org/linux-mmots.git/master
> > > compiler: gcc (GCC) 7.1.1 20170620
> > > .config is attached
> > > Raw console output is attached.
> > >
> > > syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> > > for information about syzkaller reproducers
> > >
> > >
> > > skbuff: skb_over_panic: text:0000000010b86b8d len:196 put:20
> > > head:000000003b477e60 data:000000000e85441e tail:0xd4 end:0xc0
> > > dev:lo
> > > ------------[ cut here ]------------
> > > kernel BUG at net/core/skbuff.c:104!
> > > invalid opcode: 0000 [#1] SMP KASAN
> > > Dumping ftrace buffer:
> > > (ftrace buffer empty)
> > > Modules linked in:
> > > CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.15.0-rc2-mm1+ #39
> > > Hardware name: Google Google Compute Engine/Google Compute
> > > Engine, BIOS
> > > Google 01/01/2011
> > > RIP: 0010:skb_panic+0x15c/0x1f0 net/core/skbuff.c:100
> > > RSP: 0018:ffff8801db307508 EFLAGS: 00010286
> > > RAX: 0000000000000082 RBX: ffff8801c517e840 RCX: 0000000000000000
> > > RDX: 0000000000000082 RSI: 1ffff1003b660e61 RDI: ffffed003b660e95
> > > RBP: ffff8801db307570 R08: 1ffff1003b660e23 R09: 0000000000000000
> > > R10: 0000000000000000 R11: 0000000000000000 R12: ffffffff85bd4020
> > > R13: ffffffff84754ed2 R14: 0000000000000014 R15: ffff8801c4e26540
> > > FS: 0000000000000000(0000) GS:ffff8801db300000(0000)
> > > knlGS:0000000000000000
> > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 0000000000463610 CR3: 00000001c6698000 CR4: 00000000001406e0
> > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > Call Trace:
> > > <IRQ>
> > > skb_over_panic net/core/skbuff.c:109 [inline]
> > > skb_put+0x181/0x1c0 net/core/skbuff.c:1694
> > > add_grhead.isra.24+0x42/0x3b0 net/ipv6/mcast.c:1695
> > > add_grec+0xa55/0x1060 net/ipv6/mcast.c:1817
> > > mld_send_cr net/ipv6/mcast.c:1903 [inline]
> > > mld_ifc_timer_expire+0x4d2/0x770 net/ipv6/mcast.c:2448
> > > call_timer_fn+0x23b/0x840 kernel/time/timer.c:1320
> > > expire_timers kernel/time/timer.c:1357 [inline]
> > > __run_timers+0x7e1/0xb60 kernel/time/timer.c:1660
> > > run_timer_softirq+0x4c/0xb0 kernel/time/timer.c:1686
> > > __do_softirq+0x29d/0xbb2 kernel/softirq.c:285
> > > invoke_softirq kernel/softirq.c:365 [inline]
> > > irq_exit+0x1d3/0x210 kernel/softirq.c:405
> > > exiting_irq arch/x86/include/asm/apic.h:540 [inline]
> > > smp_apic_timer_interrupt+0x16b/0x700
> > > arch/x86/kernel/apic/apic.c:1052
> > > apic_timer_interrupt+0xa9/0xb0 arch/x86/entry/entry_64.S:920
> > > </IRQ>
> > > RIP: 0010:native_safe_halt+0x6/0x10
> > > arch/x86/include/asm/irqflags.h:54
> > > RSP: 0018:ffff8801d9f97da8 EFLAGS: 00000282 ORIG_RAX:
> > > ffffffffffffff11
> > > RAX: dffffc0000000000 RBX: 1ffff1003b3f2fb8 RCX: 0000000000000000
> > > RDX: 1ffffffff0c59734 RSI: 0000000000000001 RDI: ffffffff862cb9a0
> > > RBP: ffff8801d9f97da8 R08: 0000000000000000 R09: 0000000000000000
> > > R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
> > > R13: ffff8801d9f97e60 R14: ffffffff869eb920 R15: 0000000000000000
> > > arch_safe_halt arch/x86/include/asm/paravirt.h:93 [inline]
> > > default_idle+0xbf/0x430 arch/x86/kernel/process.c:355
> > > arch_cpu_idle+0xa/0x10 arch/x86/kernel/process.c:346
> > > default_idle_call+0x36/0x90 kernel/sched/idle.c:98
> > > cpuidle_idle_call kernel/sched/idle.c:156 [inline]
> > > do_idle+0x24a/0x3b0 kernel/sched/idle.c:246
> > > cpu_startup_entry+0x18/0x20 kernel/sched/idle.c:351
> > > start_secondary+0x330/0x460 arch/x86/kernel/smpboot.c:277
> > > secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:237
> > > Code: 03 0f b6 04 01 84 c0 74 04 3c 03 7e 20 8b 4b 78 41 57 48 c7
> > > c7 a0 38
> > > bd 85 52 56 4c 89 ea 41 50 4c 89 e6 45 89 f0 e8 0c b6 3d fd <0f>
> > > 0b 4c 89 4d
> > > b8 4c 89 45 c0 48 89 75 c8 48 89 55 d0 e8 7d 93
> > > RIP: skb_panic+0x15c/0x1f0 net/core/skbuff.c:100 RSP:
> > > ffff8801db307508
> > > ---[ end trace 941a8a0f633e271f ]---
> > >
> >
> > This isn't a sctp problem, but mld's, seems when lo's mtu became 0,
> > it allocs a skb without enough space in add_grec():
> > if (AVAILABLE(skb) < sizeof(*psrc) +
> > first*sizeof(struct mld2_grec)) {
> > if (truncate && !first)
> > break; /* truncate these */
> > if (pgr)
> > pgr->grec_nsrcs = htons(scount);
> > if (skb)
> > mld_sendpack(skb);
> > skb = mld_newpack(idev, dev->mtu); <---
> >
> > I will check this for sure later on both igmp and mld.
>
> Fix:
> --- a/net/ipv6/mcast.c
> +++ b/net/ipv6/mcast.c
> @@ -1766,8 +1766,8 @@ static struct sk_buff *add_grec(struct sk_buff
> *skb, struct ifmcaddr6 *pmc,
> if (isquery)
> psf->sf_gsresp = 0;
>
> - if (AVAILABLE(skb) < sizeof(*psrc) +
> - first*sizeof(struct mld2_grec)) {
> + if (AVAILABLE(skb) < (int)(sizeof(*psrc) +
> + first * sizeof(*pgr))) {
> if (truncate && !first)
> break; /* truncate these */
> if (pgr)
> @@ -1810,7 +1810,7 @@ static struct sk_buff *add_grec(struct sk_buff
> *skb, struct ifmcaddr6 *pmc,
> return skb;
> if (pmc->mca_crcount || isquery || crsend) {
> /* make sure we have room for group header */
> - if (skb && AVAILABLE(skb) < sizeof(struct mld2_grec)) {
> + if (skb && AVAILABLE(skb) < (int)sizeof(*pgr)) {
> mld_sendpack(skb);
> skb = NULL; /* add_grhead will get a new one */
> }
>
> do the same on igmp.
Thanks for the tentative patch.
Quite a hack if you ask me.
I would rather :
1) Read dev->mtu once to avoid bad assumptions/surprises.
2) Give up if this mtu is too small for IPV6 to be functional.
Something like :
net/ipv6/mcast.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index fc6d7d143f2c29aab9a3f56eae02e5337e65a97b..844642682b8363c4c32d329ed92474f834a59618 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1682,16 +1682,16 @@ static int grec_size(struct ifmcaddr6 *pmc, int type, int gdel, int sdel)
}
static struct sk_buff *add_grhead(struct sk_buff *skb, struct ifmcaddr6 *pmc,
- int type, struct mld2_grec **ppgr)
+ int type, struct mld2_grec **ppgr, unsigned int mtu)
{
- struct net_device *dev = pmc->idev->dev;
struct mld2_report *pmr;
struct mld2_grec *pgr;
- if (!skb)
- skb = mld_newpack(pmc->idev, dev->mtu);
- if (!skb)
- return NULL;
+ if (!skb) {
+ skb = mld_newpack(pmc->idev, mtu);
+ if (!skb)
+ return NULL;
+ }
pgr = skb_put(skb, sizeof(struct mld2_grec));
pgr->grec_type = type;
pgr->grec_auxwords = 0;
@@ -1714,10 +1714,15 @@ static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,
struct mld2_grec *pgr = NULL;
struct ip6_sf_list *psf, *psf_next, *psf_prev, **psf_list;
int scount, stotal, first, isquery, truncate;
+ unsigned int mtu;
if (pmc->mca_flags & MAF_NOREPORT)
return skb;
+ mtu = READ_ONCE(dev->mtu);
+ if (mtu < IPV6_MIN_MTU)
+ return skb;
+
isquery = type == MLD2_MODE_IS_INCLUDE ||
type == MLD2_MODE_IS_EXCLUDE;
truncate = type == MLD2_MODE_IS_EXCLUDE ||
@@ -1738,7 +1743,7 @@ static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,
AVAILABLE(skb) < grec_size(pmc, type, gdeleted, sdeleted)) {
if (skb)
mld_sendpack(skb);
- skb = mld_newpack(idev, dev->mtu);
+ skb = mld_newpack(idev, mtu);
}
}
first = 1;
@@ -1774,12 +1779,12 @@ static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,
pgr->grec_nsrcs = htons(scount);
if (skb)
mld_sendpack(skb);
- skb = mld_newpack(idev, dev->mtu);
+ skb = mld_newpack(idev, mtu);
first = 1;
scount = 0;
}
if (first) {
- skb = add_grhead(skb, pmc, type, &pgr);
+ skb = add_grhead(skb, pmc, type, &pgr, mtu);
first = 0;
}
if (!skb)
@@ -1814,7 +1819,7 @@ static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,
mld_sendpack(skb);
skb = NULL; /* add_grhead will get a new one */
}
- skb = add_grhead(skb, pmc, type, &pgr);
+ skb = add_grhead(skb, pmc, type, &pgr, mtu);
}
}
if (pgr)
^ permalink raw reply related
* Re: [PATCH net-next 2/2] veth: set peer GSO values
From: David Ahern @ 2017-12-09 16:41 UTC (permalink / raw)
To: Stephen Hemminger, davem; +Cc: netdev, Stephen Hemminger
In-Reply-To: <20171207234020.18783-3-sthemmin@microsoft.com>
On 12/7/17 4:40 PM, Stephen Hemminger wrote:
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index f5438d0978ca..a69ad39ee57e 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -410,6 +410,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
> if (ifmp && (dev->ifindex != 0))
> peer->ifindex = ifmp->ifi_index;
>
> + peer->gso_max_size = dev->gso_max_size;
> + peer->gso_max_segs = dev->gso_max_segs;
> +
> err = register_netdevice(peer);
> put_net(net);
> net = NULL;
>
What if gso changes are made after device create? They are not
propagated to the peer device like they are on link create.
^ permalink raw reply
* Re: [PATCH iproute2 net-next 0/4] Abstract columns, properly space and wrap fields
From: David Ahern @ 2017-12-09 16:36 UTC (permalink / raw)
To: Stephen Hemminger, Stefano Brivio; +Cc: netdev, Sabrina Dubroca
In-Reply-To: <20171208102949.0d21cbfd@xeon-e3>
On 12/8/17 11:29 AM, Stephen Hemminger wrote:
>> - 80 columns terminal, ss -Z -f netlink
>> * before:
>> Recv-Q Send-Q Local Address:Port Peer Address:Port
>>
>> 0 0 rtnl:evolution-calen/2075 * pr
>> oc_ctx=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>> 0 0 rtnl:abrt-applet/32700 * pr
>> oc_ctx=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>> 0 0 rtnl:firefox/21619 * pr
>> oc_ctx=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>> 0 0 rtnl:evolution-calen/32639 * p
>> roc_ctx=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>> [...]
>>
>> * after:
>> Recv-Q Send-Q Local Address:Port Peer Address:Port
>> 0 0 rtnl:evolution-calen/2075 *
>> proc_ctx=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>> 0 0 rtnl:abrt-applet/32700 *
>> proc_ctx=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>> 0 0 rtnl:firefox/21619 *
>> proc_ctx=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>> 0 0 rtnl:evolution-calen/32639 *
>> proc_ctx=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>> [...]
>>
>> - 80 colums terminal, ss -tunpl
>> * before:
>> Netid State Recv-Q Send-Q Local Address:Port Peer Address:Port
>> udp UNCONN 0 0 *:37732 *:*
>> udp UNCONN 0 0 *:5353 *:*
>> udp UNCONN 0 0 192.168.122.1:53 *:*
>> udp UNCONN 0 0 *%virbr0:67 *:*
>> [...]
>>
>> * after:
>> Netid State Recv-Q Send-Q Local Address:Port Peer Address:Port
>> udp UNCONN 0 0 *:37732 *:*
>> udp UNCONN 0 0 *:5353 *:*
>> udp UNCONN 0 0 192.168.122.1:53 *:*
>> udp UNCONN 0 0 *%virbr0:67 *:*
>> [...]
>>
>> - 66 columns terminal, ss -tunpl
>> * before:
>> Netid State Recv-Q Send-Q Local Address:Port P
>> eer Address:Port
>> udp UNCONN 0 0 *:37732 *:*
>>
>> udp UNCONN 0 0 *:5353 *:*
>>
>> udp UNCONN 0 0 192.168.122.1:53
>> *:*
>> udp UNCONN 0 0 *%virbr0:67 *:*
>> [...]
>>
>> * after:
>> Netid State Recv-Q Send-Q Local Address:Port Peer Address:Port
>> udp UNCONN 0 0 *:37732 *:*
>> udp UNCONN 0 0 *:5353 *:*
>> udp UNCONN 0 0 192.168.122.1:53 *:*
>> udp UNCONN 0 0 *%virbr0:67 *:*
>> [...]
>>
>
>
> This looks good, would like some acknowledgment from heavy users such as Google
> that this works for them.
>
I'm not a Google'r but 'ss' output has annoyed me for some time, and it
has been on my to-do list to fix it. Thanks for doing that Stefano.
^ permalink raw reply
* Re: [PATCH net-next] libbpf: add function to setup XDP
From: David Ahern @ 2017-12-09 16:34 UTC (permalink / raw)
To: Eric Leblond, netdev; +Cc: linux-kernel, ast, daniel
In-Reply-To: <20171209144315.25890-1-eric@regit.org>
On 12/9/17 7:43 AM, Eric Leblond wrote:
> + /* started nested attribute for XDP */
> + nla = (struct nlattr *)(((char *)&req)
> + + NLMSG_ALIGN(req.nh.nlmsg_len));
> + nla->nla_type = NLA_F_NESTED | 43/*IFLA_XDP*/;
as a part of the move into libbpf can the magic numbers be replaced by
the names directly and there as a comment?
There are more below.
> + nla->nla_len = NLA_HDRLEN;
> +
> + /* add XDP fd */
> + nla_xdp = (struct nlattr *)((char *)nla + nla->nla_len);
> + nla_xdp->nla_type = 1/*IFLA_XDP_FD*/;
> + nla_xdp->nla_len = NLA_HDRLEN + sizeof(int);
> + memcpy((char *)nla_xdp + NLA_HDRLEN, &fd, sizeof(fd));
> + nla->nla_len += nla_xdp->nla_len;
> +
> + /* if user passed in any flags, add those too */
> + if (flags) {
> + nla_xdp = (struct nlattr *)((char *)nla + nla->nla_len);
> + nla_xdp->nla_type = 3/*IFLA_XDP_FLAGS*/;
> + nla_xdp->nla_len = NLA_HDRLEN + sizeof(flags);
> + memcpy((char *)nla_xdp + NLA_HDRLEN, &flags, sizeof(flags));
> + nla->nla_len += nla_xdp->nla_len;
> + }
> +
^ permalink raw reply
* Re: [PATCH v3 net-next 4/4] bpftool: implement cgroup bpf operations
From: David Ahern @ 2017-12-09 16:28 UTC (permalink / raw)
To: Jakub Kicinski, Roman Gushchin
Cc: netdev, linux-kernel, kernel-team, ast, daniel, kafai,
Quentin Monnet
In-Reply-To: <20171208154629.20ace049@cakuba.netronome.com>
On 12/8/17 4:46 PM, Jakub Kicinski wrote:
>> + argc -= 2;
>> + argv = &argv[2];
>> + prog_fd = prog_parse_fd(&argc, &argv);
>> + if (prog_fd < 0)
>> + goto exit_cgroup;
>> +
>> + for (i = 0; i < argc; i++) {
>> + if (strcmp(argv[i], "allow_multi") == 0) {
>> + attach_flags |= BPF_F_ALLOW_MULTI;
>> + } else if (strcmp(argv[i], "allow_override") == 0) {
>> + attach_flags |= BPF_F_ALLOW_OVERRIDE;
>
> I don't feel about this strongly but as I said I was trying to follow
> iproute2's conventions, and it allows aliasing. So if you type "ip a"
> it will give you the first thing that starts with a, not necessarily
> alphabetically, more likely in order of usefulness or order in which
> things were added. IOW if "allow_" selects "allow_mutli" that's what I
> would actually expect it to do..
>
> Maybe others disagree?
The iproute2 syntax is very user friendly, and I agree with following
the conventions.
With respect to the attach flags, allow_yyyyy is a lot to type, but
having 'a .. allow_' mean one flag over the other is going to be
confusing. Perhaps dropping the 'allow_' prefix in favor of just 'multi'
and 'override' and doing prefix match on it? User commands do not need
to follow flag names precisely.
^ permalink raw reply
* Re: Incorrect source IP address on IGMP membership report
From: Andrew Lunn @ 2017-12-09 16:01 UTC (permalink / raw)
To: Kevin Cernekee; +Cc: netdev
In-Reply-To: <CAJzqFtbXdrFYmRj7r1CF+1jH0KcH8EVG-yVdF3yMcAdzHWL5_A@mail.gmail.com>
On Fri, Dec 08, 2017 at 09:25:58PM -0800, Kevin Cernekee wrote:
> Closing a multicast socket after the final IPv4 address is deleted
> from an interface will generate a membership report that uses the
> source IP from a different interface. The following test script, run
> from an isolated netns, reproduces the issue:
>
> #!/bin/bash
>
> ip link add dummy0 type dummy
> ip link add dummy1 type dummy
> ip link set dummy0 up
> ip link set dummy1 up
> ip addr add 10.1.1.1/24 dev dummy0
> ip addr add 192.168.99.99/24 dev dummy1
>
> tcpdump -U -i dummy0 -w dummy0.pcap &
> socat EXEC:"sleep 2"
> UDP4-DATAGRAM:239.101.1.68:8889,ip-add-membership=239.0.1.68:10.1.1.1
> &
>
> sleep 1
> ip addr del 10.1.1.1/24 dev dummy0
> sleep 5
> kill %tcpdump
>
> After running this script, dummy0.pcap contains one Membership Report
> / Join Group packet with source IP 10.1.1.1, and two Membership Report
> / Leave Group packets with source IP 192.168.99.99.
>
> Sending out multicasts on the LAN using an unexpected source IP
> address seems to be causing issues in some enterprise environments[0],
> where the network infrastructure is set up to flag suspicious packets.
>
> I believe the source address is provided by ip_route_output_ports()
> called from igmpv3_newpack() in the kernel.
>
> Is this behavior intentional? If not, is it something that we should fix?
Hi Kevin
The choice of IP address for IGMP in Linux is 'interesting'. Try with
multiple IP addresses on the interfaces, addresses with different
scopes, etc. I've seen it reply to the querier using an address from
a different subnet to the incoming request, etc.
Part of it is an implementation problem. When the application did a
join, it passed an IP address to identify the interface to perform the
join on. That IP address would be an idle choice for IGMP for that
group. However, the information gets discard once the interface has
been determined.
With a single IP address on a single interface, Linux IGMP probably
works. Outside of that, expect oddness.
In your particular case, it is a global scope address. You are allowed
to use it on any interface. So it should not really trigger suspicious
activity. However, the RFC about multicast suggests IGMP with an
unexpected source address should be dropped. However, it is only a
should, not a must, if i remember correctly.
Andrew
^ 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