* Re: [Bridge] [RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled
From: Linus Lüssing @ 2019-02-19 15:42 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: bridge, netdev, roopa, f.fainelli, idosch
In-Reply-To: <7a95e759-12dd-7e7b-f2ce-f9e7f3d473f2@cumulusnetworks.com>
On Tue, Feb 19, 2019 at 03:31:42PM +0200, Nikolay Aleksandrov wrote:
> On 19/02/2019 11:21, Linus Lüssing wrote:
> > On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote:
> >> On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote:
> >>> This is v2 of the RFC patch which aims to forward packets to known
> >>> mdsts' ports only (the no querier case). After v1 I've kept
> >>> the previous behaviour when it comes to unregistered traffic or when
> >>> a querier is present. All of this is of course only with snooping
> >>> enabled. So with this patch the following changes should occur:
> >>> - No querier: forward known mdst traffic to its registered ports,
> >>> no change about unknown mcast (flood)
> >>> - Querier present: no change
> >>>
> >>> The reason to do this is simple - we want to respect the user's mdb
> >>> configuration in both cases, that is if the user adds static mdb entries
> >>> manually then we should use that information about forwarding traffic.
> >>>
> >>> What do you think ?
> >>>
> >>> * Notes
> >>> Traffic that is currently marked as mrouters_only:
> >>> - IPv4: non-local mcast traffic, igmp reports
> >>> - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports
> >>>
> >>> Simple use case:
> >>> $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
> >>> $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
> >>> - without a querier currently traffic for 239.0.0.1 will still be flooded,
> >>> with this change it will be forwarded only to swp1
> >>
> >> There is still the issue with unsolicited reports adding mdst
> >> entries here, too. Leading to unwanted packet loss and connectivity issues.
> >
> > Or in other words, an unsolicited report will turn a previously
> > unregistered multicast group into a registered one. However in the
> > absence of a querier the knowledge about this newly registered multicast group
> > will be incomplete. And therefore still needs to be flooded to avoid packet
> > loss.
> >
>
> Right, this is expected. If the user has enabled igmp snooping and doesn't have
> a querier present then such behaviour is to be expected.
IGMP snooping is currently enabled by default. And IGMP/MLD
querier is disabled by default. I wouldn't want packet loss to be
the expected behaviour in a default setup.
This default setup currently works. However with this change it
will introduce packet loss, as you acknowledged (if I understood
you correctly?).
> What is surprising is
> the user explicitly enabling igmp snooping, adding an mdst and then still
> getting it flooded. :)
Hm, for me that does not seem surprising. I would not expect igmp
snooping to work without a querier on the link. Why don't you just
add/enable a querier in your setups then if you want to avoid
flooding?
> An alternative is to drop all unregistered traffic when a querier is not present.
> But that will surely break setups and at best should be a configurable option that
> is disabled by default.
Absolutely right. Always dropping with no querier is no option. That's why I'd say
you should always flood multicast packets if there is no querier.
> So in effect and to try and make everybody happy we can add an option to control
> this behaviour with keeping the current as default and adding the following options:
> - no querier: flood all (default, current)
ACK
For the other options maybe I do not understand your scenario yet.
Wouldn't these two options result in eratic behaviour?
> - no querier: flood unregistered, forward registered
> - no querier: drop unregistered, forward registered
Let's call these two cases A) - flood unregistered, forward
registered and B) - drop unregistered, forward registered.
Let's say you have a bridge with two ports:
(1)<-[br]->(2). And no querier.
Behind (2) there is a listener for group M. M is not
registered by the bridge because either that listener joined
before the bridge came up or the entry was registered once but had
timed out. Or there was packet loss and the report did not arrive
at the bridge (for instance bc. listener is behind a wireless
connection).
For case B) we can immediately see that the listener at (2) will
not receive the traffic it signed up for. And this is a permanent
issue as the listener will not send any further reports.
Case A) is ok, the listener behind port (2) receives its traffic.
Now, a listener for M joins at (1). It sends an unsolicited
report. Group M becomes registered by the bridge. Both for
cases (A) and (B) this new listener at (1) will receive its
traffic. However, not only in case B) now, but in case A), too,
the listener at (2) will rceive no more traffic for M.
Now 260 seconds pass (multicast_membership_interval). The entry
for M times out and is deleted on the bridge. It becomes
unregistered.
Now for case (A) things would be ok again, both listeners at (1)
and (2) would receive traffic. For now - as long as no new listener
joins again.
For case (B), both the listener at port (1) and (2) will fail to
receive the traffic they signed up for.
---
I hope this illustrates a bit what I'm afraid of? If you have any
measures to prevent such behavior in your setup, I'd be curious to
know.
Regards, Linus
^ permalink raw reply
* Re: [PATCH net-next 1/3] net: dsa: add support for bridge flags
From: Vivien Didelot @ 2019-02-19 15:42 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Florian Fainelli, Andrew Lunn, David S. Miller, netdev
In-Reply-To: <20190218112355.gxroatrcaigo5nqh@shell.armlinux.org.uk>
Hi Russell,
On Mon, 18 Feb 2019 11:23:55 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> On Mon, Feb 18, 2019 at 12:50:31AM +0000, Russell King - ARM Linux admin wrote:
> > From what I can see, the port_vlan_add()/port_vlan_del() implementation
> > is far from ideal, just like "always enabling flooding for CPU/DSA
> > ports" is not ideal.
>
> There also seems to be a discrepency between what net/dsa wants to do
> and some of the implementations in drivers/net/dsa:
>
> dsa_switch_vlan_add() does this:
>
> bitmap_zero(ds->bitmap, ds->num_ports);
> if (ds->index == info->sw_index)
> set_bit(info->port, ds->bitmap);
> for (port = 0; port < ds->num_ports; port++)
> if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> set_bit(port, ds->bitmap);
>
> We then call ds->ops->port_vlan_add() for each port in ds->bitmap,
> which will include DSA and CPU ports on every switch in the tree.
>
> For rtl8366, this calls into rtl8366_vlan_add(), which does:
>
> if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
> dev_err(smi->dev, "port is DSA or CPU port\n");
>
> which is surely a guaranteed error message if we have any CPU or DSA
> ports specified on a rtl8366. The example in the DT documentation
> for this driver does suggest that it is capable of having CPU ports.
Correct, at the very beginning, most of the logic was implemented by the DSA
drivers themselves, like programming DSA and CPU ports. But this makes them
more complicated for no value, so the logic is slowly moved up to the core,
which now handles more of the generic logic.
Ideally I would like DSA drivers to be dumb, implementing simple switch-wide
ops reflecting their specs, e.g. switch-wide vlan_add ops taking a bitmap
of ports for a given VID.
This is a work in progress and we are slowly getting there. In the meantime
there are indeed discrepencies between DSA core and drivers as you pointed out.
Thanks,
Vivien
^ permalink raw reply
* [PATCH] sky2: Increase D3 delay again
From: Kai-Heng Feng @ 2019-02-19 15:45 UTC (permalink / raw)
To: mlindner, stephen, davem; +Cc: netdev, linux-kernel, Kai-Heng Feng, stable
Another platform requires even longer delay to make the device work
correctly after S3.
So increase the delay to 300ms.
BugLink: https://bugs.launchpad.net/bugs/1798921
Cc: stable@vger.kernel.org
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
drivers/net/ethernet/marvell/sky2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index f3a5fa84860f..57727fe1501e 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -5073,7 +5073,7 @@ static int sky2_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
INIT_WORK(&hw->restart_work, sky2_restart);
pci_set_drvdata(pdev, hw);
- pdev->d3_delay = 200;
+ pdev->d3_delay = 300;
return 0;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next] bpf: add skb->queue_mapping write access from tc clsact
From: Jesper Dangaard Brouer @ 2019-02-19 15:57 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, Daniel Borkmann, Alexei Starovoitov, brouer
In-Reply-To: <11d2572e-3ff5-2fc0-8f05-c50dd0fb1d6d@iogearbox.net>
On Tue, 19 Feb 2019 12:46:57 +0100
Daniel Borkmann <daniel@iogearbox.net> wrote:
> > Due to lack of TC examples, lets show howto attach clsact BPF programs:
> >
> > # tc qdisc add dev ixgbe2 clsact
> > # tc filter replace dev ixgbe2 egress bpf da obj XXX_kern.o sec tc_qmap2cpu
> > # tc filter list dev ixgbe2 egress
Recommending the "replace" is wrong is seems, as does not replace the
existing, but keeps adding more filter entries.
What is the recommended procedure for unloading and loading a newer
version of the BPF TC program?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net] sk_msg: Keep reference on socket file while psock lives
From: Daniel Borkmann @ 2019-02-19 16:00 UTC (permalink / raw)
To: Jakub Sitnicki; +Cc: netdev, John Fastabend, Marek Majkowski
In-Reply-To: <20190211090949.18560-1-jakub@cloudflare.com>
Hi Jakub,
On 02/11/2019 10:09 AM, Jakub Sitnicki wrote:
> Backlog work for psock (sk_psock_backlog) might sleep while waiting for
> memory to free up when sending packets. While sleeping, socket can
> disappear from under our feet together with its wait queue because the
> userspace has closed it.
>
> This breaks an assumption in sk_stream_wait_memory, which expects the
> wait queue to be still there when it wakes up resulting in a
> use-after-free:
>
> ==================================================================
> BUG: KASAN: use-after-free in remove_wait_queue+0x31/0x70
> Write of size 8 at addr ffff888069a0c4e8 by task kworker/0:2/110
>
> CPU: 0 PID: 110 Comm: kworker/0:2 Not tainted 5.0.0-rc2-00335-g28f9d1a3d4fe-dirty #14
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
> Workqueue: events sk_psock_backlog
> Call Trace:
> print_address_description+0x6e/0x2b0
> ? remove_wait_queue+0x31/0x70
> kasan_report+0xfd/0x177
> ? remove_wait_queue+0x31/0x70
> ? remove_wait_queue+0x31/0x70
> remove_wait_queue+0x31/0x70
> sk_stream_wait_memory+0x4dd/0x5f0
> ? sk_stream_wait_close+0x1b0/0x1b0
> ? wait_woken+0xc0/0xc0
> ? tcp_current_mss+0xc5/0x110
> tcp_sendmsg_locked+0x634/0x15d0
> ? tcp_set_state+0x2e0/0x2e0
> ? __kasan_slab_free+0x1d1/0x230
> ? kmem_cache_free+0x70/0x140
> ? sk_psock_backlog+0x40c/0x4b0
> ? process_one_work+0x40b/0x660
> ? worker_thread+0x82/0x680
> ? kthread+0x1b9/0x1e0
> ? ret_from_fork+0x1f/0x30
> ? check_preempt_curr+0xaf/0x130
> ? iov_iter_kvec+0x5f/0x70
> ? kernel_sendmsg_locked+0xa0/0xe0
> skb_send_sock_locked+0x273/0x3c0
> ? skb_splice_bits+0x180/0x180
> ? start_thread+0xe0/0xe0
> ? update_min_vruntime.constprop.27+0x88/0xc0
> sk_psock_backlog+0xb3/0x4b0
> ? strscpy+0xbf/0x1e0
> process_one_work+0x40b/0x660
> worker_thread+0x82/0x680
> ? process_one_work+0x660/0x660
> kthread+0x1b9/0x1e0
> ? __kthread_create_on_node+0x250/0x250
> ret_from_fork+0x1f/0x30
>
> Allocated by task 109:
> sock_alloc_inode+0x54/0x120
> alloc_inode+0x28/0xb0
> new_inode_pseudo+0x7/0x60
> sock_alloc+0x21/0xe0
> __sys_accept4+0xc2/0x330
> __x64_sys_accept+0x3b/0x50
> do_syscall_64+0xb2/0x3e0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Freed by task 7:
> kfree+0x7f/0x140
> rcu_process_callbacks+0xe0/0x100
> __do_softirq+0xe5/0x29a
>
> The buggy address belongs to the object at ffff888069a0c4e0
> which belongs to the cache kmalloc-64 of size 64
> The buggy address is located 8 bytes inside of
> 64-byte region [ffff888069a0c4e0, ffff888069a0c520)
> The buggy address belongs to the page:
> page:ffffea0001a68300 count:1 mapcount:0 mapping:ffff88806d4018c0 index:0x0
> flags: 0x4000000000000200(slab)
> raw: 4000000000000200 dead000000000100 dead000000000200 ffff88806d4018c0
> raw: 0000000000000000 00000000002a002a 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff888069a0c380: fb fb fb fb fc fc fc fc fb fb fb fb fb fb fb fb
> ffff888069a0c400: fc fc fc fc fb fb fb fb fb fb fb fb fc fc fc fc
>> ffff888069a0c480: 00 00 00 00 00 00 00 00 fc fc fc fc fb fb fb fb
> ^
> ffff888069a0c500: fb fb fb fb fc fc fc fc fb fb fb fb fb fb fb fb
> ffff888069a0c580: fc fc fc fc fb fb fb fb fb fb fb fb fc fc fc fc
> ==================================================================
>
> Avoid it by keeping a reference to the socket file until the psock gets
> destroyed.
>
> While at it, rearrange the order of reference grabbing and
> initialization to match the destructor in reverse.
>
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Link: https://lore.kernel.org/netdev/CAJPywTLwgXNEZ2dZVoa=udiZmtrWJ0q5SuBW64aYs0Y1khXX3A@mail.gmail.com
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
> net/core/skmsg.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 8c826603bf36..a38442b8580b 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -493,8 +493,13 @@ struct sk_psock *sk_psock_init(struct sock *sk, int node)
> sk_psock_set_state(psock, SK_PSOCK_TX_ENABLED);
> refcount_set(&psock->refcnt, 1);
>
> - rcu_assign_sk_user_data(sk, psock);
> + /* Hold on to socket wait queue. Backlog work waits on it for
> + * memory when sending. We must cancel work before socket wait
> + * queue can go away.
> + */
> + get_file(sk->sk_socket->file);
> sock_hold(sk);
> + rcu_assign_sk_user_data(sk, psock);
>
> return psock;
> }
> @@ -558,6 +563,7 @@ static void sk_psock_destroy_deferred(struct work_struct *gc)
> if (psock->sk_redir)
> sock_put(psock->sk_redir);
> sock_put(psock->sk);
> + fput(psock->sk->sk_socket->file);
Thanks for the report (and sorry for the late reply). I think holding ref on
the struct file just so we keep it alive till deferred destruction might be
papering over the actual underlying bug. Nothing obvious pops out from staring
at the code right now; as a reproducer to run, did you use the prog in the link
above and hit it after your strparser fix?
Thanks again,
Daniel
^ permalink raw reply
* Re: [PATCH net-next v4 05/17] net: sched: traverse chains in block with tcf_get_next_chain()
From: Vlad Buslov @ 2019-02-19 16:04 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <CAM_iQpVCP=6f4iRVqbgHxZcNHgmsdDJmuUQLk-9uPZar2xTGfw@mail.gmail.com>
On Mon 18 Feb 2019 at 18:26, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Feb 18, 2019 at 2:07 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> Hi Cong,
>>
>> Thanks for reviewing!
>>
>> On Fri 15 Feb 2019 at 22:21, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > (Sorry for joining this late.)
>> >
>> > On Mon, Feb 11, 2019 at 12:56 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >> @@ -2432,7 +2474,11 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
>> >> index_start = cb->args[0];
>> >> index = 0;
>> >>
>> >> - list_for_each_entry(chain, &block->chain_list, list) {
>> >> + for (chain = __tcf_get_next_chain(block, NULL);
>> >> + chain;
>> >> + chain_prev = chain,
>> >> + chain = __tcf_get_next_chain(block, chain),
>> >> + tcf_chain_put(chain_prev)) {
>> >
>> > Why do you want to take the block->lock in each iteration
>> > of the loop rather than taking once for the whole loop?
>>
>> This loop calls classifier ops callback in tc_chain_fill_node(). I don't
>> call any classifier ops callbacks while holding block or chain lock in
>> this change because the goal is to achieve fine-grained locking for data
>> structures used by filter update path. Locking per-block or per-chain is
>> much coarser than taking reference counters to parent structures and
>> allowing classifiers to implement their own locking.
>
> That is the problem, when we have N filter chains in a block, you
> lock and unlock mutex N times... And what __tcf_get_next_chain()
> does is basically just retrieving the next entry in the list, so the
> overhead of mutex is likely more than the list operation itself in
> contention situation.
>
> Now I can see why you complained about mutex before, it is
> how you use it, not actually its own problem. :)
>
>>
>> In this case call to ops->tmplt_dump() is probably quite fast and its
>> execution time doesn't depend on number of filters on the classifier, so
>> releasing block->lock on each iteration doesn't provide much benefit, if
>> at all. However, it is easier for me to reason about locking correctness
>> in this refactoring by following a simple rule that no locks (besides
>> rtnl mutex) can be held when calling classifier ops callbacks.
>
> Well, for me, a hierarchy locking is always simple when you take
> them in the right order, that is locking the larger-scope lock first
> and then smaller-scope one.
>
> The way you use the locking here is actually harder for me to
> review, because it is hard to valid its atomicity when you unlock
> the larger scope lock and re-take the smaller scope lock. You
> use refcnt to ensure it will not go way, but that is still far from
> guarantee of the atomicity.
>
> For example, tp->ops->change() which changes an existing
> filter, I don't see you lock either block->lock or
> chain->filter_chain_lock when calling it. How does it even work?
Sorry for not describing it in more details in cover letter. Basic
approach is that cls API obtains references to all necessary data
structures (Qdisc, block, chain, tp) and calls classifier ops callbacks
after releasing all the locks. This defers all locking and atomicity
concerns to actual classifier implementations. In case of
tp->ops->change() flower classifier uses tp->lock. In case of dump code
(both chain and filter) there is no atomicity with or without my changes
because rtnl lock is released after sending each skb to userspace and
concurrent changes to tc structures can occur in between.
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] net: dsa: mv88e6xxx: default to multicast and unicast flooding
From: Vivien Didelot @ 2019-02-19 16:05 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190218125345.eq3mhmgsmgm7jmem@shell.armlinux.org.uk>
Hi Russell,
On Mon, 18 Feb 2019 12:53:45 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> On Sun, Feb 17, 2019 at 04:32:40PM +0000, Russell King wrote:
> > Switches work by learning the MAC address for each attached station by
> > monitoring traffic from each station. When a station sends a packet,
> > the switch records which port the MAC address is connected to.
> >
> > With IPv4 networking, before communication commences with a neighbour,
> > an ARP packet is broadcasted to all stations asking for the MAC address
> > corresponding with the IPv4. The desired station responds with an ARP
> > reply, and the ARP reply causes the switch to learn which port the
> > station is connected to.
> >
> > With IPv6 networking, the situation is rather different. Rather than
> > broadcasting ARP packets, a "neighbour solicitation" is multicasted
> > rather than broadcasted. This multicast needs to reach the intended
> > station in order for the neighbour to be discovered.
> >
> > Once a neighbour has been discovered, and entered into the sending
> > stations neighbour cache, communication can restart at a point later
> > without sending a new neighbour solicitation, even if the entry in
> > the neighbour cache is marked as stale. This can be after the MAC
> > address has expired from the forwarding cache of the DSA switch -
> > when that occurs, there is a long pause in communication.
Thank you for the very informative message above.
> > Our DSA implementation for mv88e6xxx switches has defaulted to having
> > multicast and unicast flooding disabled. As per the above description,
> > this is fine for IPv4 networking, since the broadcasted ARP queries
> > will be sent to and received by all stations on the same network.
> > However, this breaks IPv6 very badly - blocking neighbour solicitations
> > and later causing connections to stall.
> >
> > The defaults that the Linux bridge code expect from bridges are that
> > unknown unicast frames and unknown multicast frames are flooded to
> > all stations, which is at odds to the defaults adopted by our DSA
> > implementation for mv88e6xxx switches.
> >
> > This commit enables by default flooding of both unknown unicast and
> > unknown multicast frames. This means that mv88e6xxx DSA switches now
> > behave as per the bridge(8) man page, and IPv6 works flawlessly through
> > such a switch.
>
> Thinking about this a bit more, this approach probably isn't the best.
> If we have a port that goes through this life-cycle:
>
> 1. assigned to a bridge
> 2. configured not to flood
> 3. reassigned to a new bridge
>
> the port will retain its settings from the first bridge, which will be
> at odds with the settings that the Linux bridge code expects and the
> settings visible to the user.
>
> So, how about this, which basically reverts this patch and applies the
> flood settings each time a port joins a bridge, and clears them when
> the port leaves a bridge.
Isn't the bridge code programming flooding on the port correctly on leave/join,
because the BR_*FLOOD flags have been learned? I would expect that.
Thanks,
Vivien
^ permalink raw reply
* Re: Handling an Extra Signal at PHY Reset
From: Florian Fainelli @ 2019-02-19 16:07 UTC (permalink / raw)
To: Paul Kocialkowski, Andrew Lunn, Heiner Kallweit
Cc: netdev, Thomas Petazzoni, Mylène Josserand
In-Reply-To: <e647050a0fbadb8445cf6a7a5a2ccfbfd0865592.camel@bootlin.com>
On February 19, 2019 1:14:20 AM PST, Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:
>Hi,
>
>We are dealing with an Ethernet PHY (Marvell 88E1512) that comes with a
>CONFIG pin that must be connected to one of the other pins of the PHY
>to configure the LSB of the PHY address as well as I/O voltages (see
>section 2.18.1 Hardware Configuration of the datasheet). It must be
>connected "soon after reset" for the PHY to be correctly configured.
Even voltage? What guarantees do you have that you are not reducing the lifetime of your pads if e.g.: you are configured for 3.3V while the other end is 1.8/2.5V? Is there some kind of embedded voltage comparator that can be used to help making the right decision?
>
>We have a switch for connecting the CONFIG pin to the other pin (LED0),
>which needs to be controlled by Linux. The CONFIG pin seems to be used
>for a PTP clock the rest of the time.
>
>So we are wondering how to properly represent this case, especially on
>the device-tree side.
>
>The trick here is that this step is necessary before the PHY can be
>discovered on the MDIO bus (and thus the PHY driver selected) so we
>can't rely on the PHY driver to do this. Basically, it looks like we
>need to handle this like the reset pin and describe it at the MDIO bus
>level.
>
>Here are some ideas for potential solutions:
>- Allowing more than a single GPIO to be passed to the MDIO bus' reset-
>gpios via device-tree and toggling all the passed GPIOs at once;
That would be a mis-representstion of the HW though, since the reset line is tied to the PHY package. Making use of the current implementation details to put a second reset line does not sound great.
>
>- Adding a new optional GPIO for the MDIO bus dedicated to controlling
>switches for such config switching, perhaps called "config-gpios"
>(quite a narrow solution);
Indeed, and still has the same design flaw as 1) outline above.
>
>- Adding a broader power sequence description to the MDIO bus (a bit
>like it's done with the mmc pwrseq descriptions) which would allow
>specifying the toggle order/delays of various GPIOs (would probably be
>the most extensive solution);
That one looks the most compelling and future proof although I do wonder how many people would make use of that.
>
>- Adding the extra GPIO control to the MAC description and toggling it
>through bus->reset (probably the less invasive solution for the core
>but not very satisfying from the description perspective, since this is
>definitely not MAC-specific).
>
>What do you think about how we could solve this issue?
>Do you see other options that I missed here?
You could explore having the MDIO bus layer scan its children nodes (PHY nodes) and handle properties in there before registering devices, so for insurance your PHY DT nodes can have an arbitrary number of reset lines, power sequencing properties etc. and the MDIO bus layer knowing it's internal implementation does make sure that it makes use of these properties in order to make PHY devices functional.
Does that make sense? One possible caveat is that the CONFIG pin also dictates the address on the bus, so what do we do with the PHY's "reg" property, is it it's actual address or is it the desired one that we should configure through reset?
--
Florian
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: add skb->queue_mapping write access from tc clsact
From: Daniel Borkmann @ 2019-02-19 16:08 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: netdev, Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <20190219165745.676097a3@carbon>
On 02/19/2019 04:57 PM, Jesper Dangaard Brouer wrote:
> On Tue, 19 Feb 2019 12:46:57 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>>> Due to lack of TC examples, lets show howto attach clsact BPF programs:
>>>
>>> # tc qdisc add dev ixgbe2 clsact
>>> # tc filter replace dev ixgbe2 egress bpf da obj XXX_kern.o sec tc_qmap2cpu
>>> # tc filter list dev ixgbe2 egress
>
> Recommending the "replace" is wrong is seems, as does not replace the
> existing, but keeps adding more filter entries.
>
> What is the recommended procedure for unloading and loading a newer
> version of the BPF TC program?
You would need to specify prio / handle in order to select a particular
instance for atomic replacement:
tc filter replace dev foo {e,in}gress prio 1 handle 1 bpf da obj foo.o
^ permalink raw reply
* Re: [PATCH net] team: use operstate consistently for linkup
From: Jiri Pirko @ 2019-02-19 16:00 UTC (permalink / raw)
To: George Wilkie; +Cc: David S. Miller, netdev
In-Reply-To: <20190219155715.769-1-gwilkie@vyatta.att-mail.com>
Tue, Feb 19, 2019 at 04:57:15PM CET, gwilkie@vyatta.att-mail.com wrote:
>When a port is added to a team, its initial state is derived
>from netif_carrier_ok rather than netif_oper_up.
>If it is carrier up but operationally down at the time of being
>added, the port state.linkup will be set prematurely.
>port state.linkup should be set consistently using
>netif_oper_up rather than netif_carrier_ok.
>
>Fixes: f1d22a1e0595 ("team: account for oper state")
>
>Signed-off-by: George Wilkie <gwilkie@vyatta.att-mail.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
^ permalink raw reply
* pull-request: ieee802154-next 2019-02-19
From: Stefan Schmidt @ 2019-02-19 16:11 UTC (permalink / raw)
To: davem; +Cc: linux-wpan, alex.aring, netdev
Hello Dave.
An update from ieee802154 for *net-next*
Another quite quite cycle in the ieee802154 subsystem.
Peter did a rework of the IP frag queue handling to make it use rbtree and get
in line with the core IPv4 and IPv6 implementatiosn in the kernel.
Please pull, or let me know if there are any problems.
regards
Stefan Schmidt
The following changes since commit c9b747dbc2036c917b1067fbb78dc38b105c4454:
bnx2x: Remove set but not used variable 'mfw_vn' (2019-02-18 16:47:32 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/sschmidt/wpan-next.git ieee802154-for-davem-2019-02-19
for you to fetch changes up to 254c5dbe15d44c9d76a3ccd3724eb1befb7adf99:
6lowpan: use rbtree for IP frag queue (2019-02-19 10:38:01 +0100)
----------------------------------------------------------------
Peter Oskolkov (1):
6lowpan: use rbtree for IP frag queue
net/ieee802154/6lowpan/reassembly.c | 141 ++++++++----------------------------
1 file changed, 29 insertions(+), 112 deletions(-)
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Vivien Didelot @ 2019-02-19 16:16 UTC (permalink / raw)
To: Russell King
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <E1gvPMo-0000Ah-Vy@rmk-PC.armlinux.org.uk>
Hi Russell,
On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
> + unsigned long flags)
> +{
> + struct mv88e6xxx_chip *chip = ds->priv;
> + bool unicast, multicast;
> + int ret = -EOPNOTSUPP;
> +
> + unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
> + flags & BR_FLOOD;
> + multicast = flags & BR_MCAST_FLOOD;
> +
> + mutex_lock(&chip->reg_lock);
> + if (chip->info->ops->port_set_egress_floods)
> + ret = chip->info->ops->port_set_egress_floods(chip, port,
> + unicast,
> + multicast);
> + mutex_unlock(&chip->reg_lock);
> +
> + return ret;
> +}
> +
> +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> +{
> + struct mv88e6xxx_chip *chip = ds->priv;
> + unsigned long support = 0;
> +
> + if (chip->info->ops->port_set_egress_floods)
> + support |= BR_FLOOD | BR_MCAST_FLOOD;
> +
> + return support;
> +}
I think that it isn't necessary to propagate the notion of bridge flags down
to the DSA drivers. It might be just enough to add:
port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] net: dsa: mv88e6xxx: default to multicast and unicast flooding
From: Russell King - ARM Linux admin @ 2019-02-19 16:18 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219110537.GD27578@t480s.localdomain>
On Tue, Feb 19, 2019 at 11:05:37AM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Mon, 18 Feb 2019 12:53:45 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > On Sun, Feb 17, 2019 at 04:32:40PM +0000, Russell King wrote:
> > > Switches work by learning the MAC address for each attached station by
> > > monitoring traffic from each station. When a station sends a packet,
> > > the switch records which port the MAC address is connected to.
> > >
> > > With IPv4 networking, before communication commences with a neighbour,
> > > an ARP packet is broadcasted to all stations asking for the MAC address
> > > corresponding with the IPv4. The desired station responds with an ARP
> > > reply, and the ARP reply causes the switch to learn which port the
> > > station is connected to.
> > >
> > > With IPv6 networking, the situation is rather different. Rather than
> > > broadcasting ARP packets, a "neighbour solicitation" is multicasted
> > > rather than broadcasted. This multicast needs to reach the intended
> > > station in order for the neighbour to be discovered.
> > >
> > > Once a neighbour has been discovered, and entered into the sending
> > > stations neighbour cache, communication can restart at a point later
> > > without sending a new neighbour solicitation, even if the entry in
> > > the neighbour cache is marked as stale. This can be after the MAC
> > > address has expired from the forwarding cache of the DSA switch -
> > > when that occurs, there is a long pause in communication.
>
> Thank you for the very informative message above.
>
> > > Our DSA implementation for mv88e6xxx switches has defaulted to having
> > > multicast and unicast flooding disabled. As per the above description,
> > > this is fine for IPv4 networking, since the broadcasted ARP queries
> > > will be sent to and received by all stations on the same network.
> > > However, this breaks IPv6 very badly - blocking neighbour solicitations
> > > and later causing connections to stall.
> > >
> > > The defaults that the Linux bridge code expect from bridges are that
> > > unknown unicast frames and unknown multicast frames are flooded to
> > > all stations, which is at odds to the defaults adopted by our DSA
> > > implementation for mv88e6xxx switches.
> > >
> > > This commit enables by default flooding of both unknown unicast and
> > > unknown multicast frames. This means that mv88e6xxx DSA switches now
> > > behave as per the bridge(8) man page, and IPv6 works flawlessly through
> > > such a switch.
> >
> > Thinking about this a bit more, this approach probably isn't the best.
> > If we have a port that goes through this life-cycle:
> >
> > 1. assigned to a bridge
> > 2. configured not to flood
> > 3. reassigned to a new bridge
> >
> > the port will retain its settings from the first bridge, which will be
> > at odds with the settings that the Linux bridge code expects and the
> > settings visible to the user.
> >
> > So, how about this, which basically reverts this patch and applies the
> > flood settings each time a port joins a bridge, and clears them when
> > the port leaves a bridge.
>
> Isn't the bridge code programming flooding on the port correctly on leave/join,
> because the BR_*FLOOD flags have been learned? I would expect that.
If you're asking whether the bridge code sends a
SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS message on leave/join, it seems
that it does not.
There is only one place in the bridge code that this message is
generated, that is in net/bridge/br_switchdev.c
br_switchdev_set_port_flag(). That is called from one place in the
bridge code, which is br_set_port_flag() in net/bridge/br_netlink.c,
which is in response to a RTM_SETLINK netlink message where the bridge
code processes all the various bridge link options.
There appears to be no call when adding or removing an interface to/
from a bridge.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: add skb->queue_mapping write access from tc clsact
From: Daniel Borkmann @ 2019-02-19 16:18 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: netdev, Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <20190219155259.677d195c@carbon>
On 02/19/2019 03:52 PM, Jesper Dangaard Brouer wrote:
> On Tue, 19 Feb 2019 12:46:57 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>> On 02/19/2019 11:24 AM, Jesper Dangaard Brouer wrote:
>>> The skb->queue_mapping already have read access, via __sk_buff->queue_mapping.
>>>
>>> This patch allow BPF tc qdisc clsact write access to the queue_mapping via
>>> tc_cls_act_is_valid_access.
>>>
>>> It is already possible to change this via TC filter action skbedit
>>> tc-skbedit(8). Due to the lack of TC examples, lets show one:
>>>
>>> # tc qdisc add dev ixgbe1 handle ffff: ingress
>>> # tc filter add dev ixgbe1 parent ffff: matchall action skbedit queue_mapping 5
>>> # tc filter list dev ixgbe1 parent ffff:
>>
>> Using handles was in the old days, if we add examples, then lets do
>> something more user friendly ;)
>>
>> # tc qdisc add dev ixgbe1 clsact
>> # tc filter replace dev ixgbe1 ingress matchall action skbedit queue_mapping 5
>> # tc filter list dev ixgbe1 ingress
>>
>>> The most common mistake is that XPS (Transmit Packet Steering) takes
>>> precedence over setting skb->queue_mapping. XPS is configured per DEVICE
>>> via /sys/class/net/DEVICE/queues/tx-*/xps_cpus via a CPU hex mask. To
>>> disable set mask=00.
>>>
>>> The purpose of changing skb->queue_mapping is to influence the selection of
>>> the net_device "txq" (struct netdev_queue), which influence selection of
>>> the qdisc "root_lock" (via txq->qdisc->q.lock) and txq->_xmit_lock. When
>>> using the MQ qdisc the txq->qdisc points to different qdiscs and associated
>>> locks, and HARD_TX_LOCK (txq->_xmit_lock), allowing for CPU scalability.
>>>
>>> Due to lack of TC examples, lets show howto attach clsact BPF programs:
>>>
>>> # tc qdisc add dev ixgbe2 clsact
>>> # tc filter replace dev ixgbe2 egress bpf da obj XXX_kern.o sec tc_qmap2cpu
>>> # tc filter list dev ixgbe2 egress
>>>
>>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>> ---
>>> net/core/filter.c | 14 +++++++++++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 353735575204..d05ae8d05397 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -6238,6 +6238,7 @@ static bool tc_cls_act_is_valid_access(int off, int size,
>>> case bpf_ctx_range(struct __sk_buff, tc_classid):
>>> case bpf_ctx_range_till(struct __sk_buff, cb[0], cb[4]):
>>> case bpf_ctx_range(struct __sk_buff, tstamp):
>>> + case bpf_ctx_range(struct __sk_buff, queue_mapping):
>>> break;
>>> default:
>>> return false;
>>> @@ -6642,9 +6643,16 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
>>> break;
>>>
>>> case offsetof(struct __sk_buff, queue_mapping):
>>> - *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
>>> - bpf_target_off(struct sk_buff, queue_mapping, 2,
>>> - target_size));
>>> + if (type == BPF_WRITE)
>>> + *insn++ = BPF_STX_MEM(BPF_H, si->dst_reg, si->src_reg,
>>> + bpf_target_off(struct sk_buff,
>>> + queue_mapping,
>>> + 2, target_size));
>>> + else
>>> + *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
>>> + bpf_target_off(struct sk_buff,
>>> + queue_mapping,
>>> + 2, target_size));
>>
>> One thing we should avoid would be to allow user to write NO_QUEUE_MAPPING
>> into skb->queue_mapping so we don't hit the warn in sk_tx_queue_set(), I'd
>> add this into the ctx rewrite here.
>
> Makes sense. I would really appreciate if you could help me out writing
> the needed BPF instructions, as I'm not an expert here.
Untested / uncompiled, but should be:
case offsetof(struct __sk_buff, queue_mapping):
if (type == BPF_WRITE) {
*insn++ = BPF_JMP_IMM(BPF_JGE, si->src_reg, NO_QUEUE_MAPPING, 1);
*insn++ = BPF_STX_MEM(BPF_H, si->dst_reg, si->src_reg,
bpf_target_off(struct sk_buff, queue_mapping, 2,
target_size));
} else {
*insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
bpf_target_off(struct sk_buff, queue_mapping, 2,
target_size));
}
break;
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Russell King - ARM Linux admin @ 2019-02-19 16:24 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219111612.GF27578@t480s.localdomain>
On Tue, Feb 19, 2019 at 11:16:12AM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> > +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
> > + unsigned long flags)
> > +{
> > + struct mv88e6xxx_chip *chip = ds->priv;
> > + bool unicast, multicast;
> > + int ret = -EOPNOTSUPP;
> > +
> > + unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
> > + flags & BR_FLOOD;
> > + multicast = flags & BR_MCAST_FLOOD;
> > +
> > + mutex_lock(&chip->reg_lock);
> > + if (chip->info->ops->port_set_egress_floods)
> > + ret = chip->info->ops->port_set_egress_floods(chip, port,
> > + unicast,
> > + multicast);
> > + mutex_unlock(&chip->reg_lock);
> > +
> > + return ret;
> > +}
> > +
> > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > +{
> > + struct mv88e6xxx_chip *chip = ds->priv;
> > + unsigned long support = 0;
> > +
> > + if (chip->info->ops->port_set_egress_floods)
> > + support |= BR_FLOOD | BR_MCAST_FLOOD;
> > +
> > + return support;
> > +}
>
> I think that it isn't necessary to propagate the notion of bridge flags down
> to the DSA drivers. It might be just enough to add:
>
> port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
>
> to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
There are two other flags that I haven't covered which the bridge code
expects to be offloaded, and those are the broadcast flood flag and
the learning flag.
I know that the Marvell switches don't have a bit to control the
broadcast flooding, that appears to be controlled via a static entry
in the ATU which would have to be modified as the broadcast flood flag
is manipulated. I don't know how that is handled in other bridges.
Do we want to include the broadcast flood in the above prototype?
If we go for this, how do we detect which options a switch supports?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH][next] vhost: only return early if ret indicates an error or no iovecs have been processed
From: Michael S. Tsirkin @ 2019-02-19 16:35 UTC (permalink / raw)
To: Colin King
Cc: Jason Wang, kvm, virtualization, netdev, kernel-janitors,
linux-kernel
In-Reply-To: <20190219135713.10426-1-colin.king@canonical.com>
On Tue, Feb 19, 2019 at 01:57:13PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently the loop that calls log_write_hva on each iovec is never
> executed because of an incorrect error check on the return from the
> call to translate_desc. The check should be checking for a -ve error
> return and because it makes no sense to iterate over zero items, the
> checks should also check for zero too.
>
> Detected by CoverityScan, CID#1476969 ("Logically dead code")
>
> Fixes: cc5e71075947 ("vhost: log dirty page correctly")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Jason posted a similar patch recently.
Are you happy with that one?
> ---
> drivers/vhost/vhost.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 24a129fcdd61..a9a1709a859a 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1788,7 +1788,7 @@ static int log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
>
> ret = translate_desc(vq, (uintptr_t)vq->used + used_offset,
> len, iov, 64, VHOST_ACCESS_WO);
> - if (ret)
> + if (ret <= 0)
> return ret;
>
> for (i = 0; i < ret; i++) {
> --
> 2.20.1
^ permalink raw reply
* Re: L2TPv3 offset
From: James Chapman @ 2019-02-19 16:36 UTC (permalink / raw)
To: t.martitz; +Cc: davem, netdev
In-Reply-To: <OFF1459E30.8430BF87-ONC12583A6.0047B081-C12583A6.004841EE@avm.de>
On 19/02/2019 13:09, t.martitz@avm.de wrote:
>
> Hello,
>
> thanks for your quick response.
>
> "James Chapman" <jchapman@katalix.com> schrieb am 19.02.2019 13:40:10:
>
> > Von: "James Chapman" <jchapman@katalix.com>
> > An: t.martitz@avm.de
> > Kopie: davem@davemloft.net, "netdev" <netdev@vger.kernel.org>
> > Datum: 19.02.2019 13:40
> > Betreff: Re: L2TPv3 offset
> >
> > On 19/02/2019 09:17, t.martitz@avm.de wrote:
> > >
> > > Hello,
> > >
> > > I saw that you removed the offset option from l2tp sessions in Linux
> > > 4.16 (commit 900631ee6a2651dc4fbaecb8ef9fa5f1e3378853 l2tp: remove
> > > configurable payload offset). Since we need something like that I'm
> > > reaching out to you.
> > >
> > Adding netdev.
> > >
> > >
> > > Our use case is pseudo-wire over l2tp, so we encapsulate ethernet
> > > within l2tpv3. This has lots of benifits for us, but one remarkable
> > > drawback is that the receiver, who's removing the encapsulation, will
> > > see a misaligned inner IP header.
> > >
> > > So we planned to use the offset to correct that. But I saw that the
> > > offset was removed not too long ago. Now I see that there is still
> > > "l2specific_len" / L2TP_ATTR_L2SPEC_LEN that we can use for pretty
> > > much the same purpose but I get the feeling that this should only be 0
> > > or 4 while we would need 2 or 6. I get this feeling because "ip l2tp"
> > > of the iproute2 package does not allow setting this at all and
> > > hardcodes 0 or 4 based on the type.
> > >
> > In L2TPv3, any payload offset would need to be implemented by defining a
> > new L2SpecificSublayer type since the L2TPv3 header has no offset field.
> > There are two standard L2TPv3 sublayers supported - None and Default.
> > These are fixed size (0 and 4 bytes respectively), hence the kernel now
> > ignores L2TP_ATTR_L2SPEC_LEN since the length can be derived from
> > L2TP_ATTR_L2SPEC_TYPE. For reference, the set of defined types are
> > listed at
> > https://www.iana.org/assignments/l2tp-parameters/l2tp-
> > parameters.xhtml#l2tp-parameters-37.
> > No L2SpecificSublayer type was ever defined for L2TPv3 to allow a
> > configurable payload offset in ethernet pseudowires.
>
>
> So are we the only ones to encapsulate Ethernet within L2TPv3? It's
> hard to believe.
>
No of course not.
>
>
> Because everytime a LCCE decapsulates such traffic it'll suffer from
> unaligned access to the inner IP header (likewise for the outer IP
> header when encapsulating). It's a fundamental assumption that the IP
> header is word-aligned in Linux so I'm surprised this isn't solved
> already. And now the only way "fix" without patching the kernel is
> being removed.
>
It was removed to prevent the possibility of sending non-compliant
L2TPv3 packets into the network. L2TPv3 allows for new
L2SpecificSublayer header types to be defined. It's a shame that one
hasn't been added to allow for this.
Are you using a CPU that can't do unaligned word accesses?
>
> >
> > So my question is what should we do know? Being based on removed
> > functionality is kind of bad, but we must fix the misaligned inner ip
> > header.
> > We control the sender and receiver so we could apply all kinds of
> > hacks, but we would rather find a solution compliant with the Internet
> > community.
> >
> If you really need to insert padding in transmitted L2TPv3 packets
> between the L2TPv3 header and its payload, one option is to define a new
> L2SpecificHeaderType and patch the kernel to accept it.
>
> If you control both the sender and receiver, is using FOU an option?
>
What does FOU refer to?
Foo-Over-UDP. See ip-fou(8).
^ permalink raw reply
* Re: [PATCH][next] vhost: only return early if ret indicates an error or no iovecs have been processed
From: Colin Ian King @ 2019-02-19 16:40 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, kvm, virtualization, netdev, kernel-janitors,
linux-kernel
In-Reply-To: <20190219113510-mutt-send-email-mst@kernel.org>
On 19/02/2019 16:35, Michael S. Tsirkin wrote:
> On Tue, Feb 19, 2019 at 01:57:13PM +0000, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Currently the loop that calls log_write_hva on each iovec is never
>> executed because of an incorrect error check on the return from the
>> call to translate_desc. The check should be checking for a -ve error
>> return and because it makes no sense to iterate over zero items, the
>> checks should also check for zero too.
>>
>> Detected by CoverityScan, CID#1476969 ("Logically dead code")
>>
>> Fixes: cc5e71075947 ("vhost: log dirty page correctly")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>
> Jason posted a similar patch recently.
>
> Are you happy with that one?
Sure, ignore mine, I was late to the party.
>
>> ---
>> drivers/vhost/vhost.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 24a129fcdd61..a9a1709a859a 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -1788,7 +1788,7 @@ static int log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
>>
>> ret = translate_desc(vq, (uintptr_t)vq->used + used_offset,
>> len, iov, 64, VHOST_ACCESS_WO);
>> - if (ret)
>> + if (ret <= 0)
>> return ret;
>>
>> for (i = 0; i < ret; i++) {
>> --
>> 2.20.1
^ permalink raw reply
* [PATCH net 0/2] report erspan version field just for erspan tunnels
From: Lorenzo Bianconi @ 2019-02-19 16:42 UTC (permalink / raw)
To: davem; +Cc: netdev, u9012063
Do not report erspan_version to userpsace for non erspan tunnels.
Report IFLA_GRE_ERSPAN_INDEX only for erspan version 1 in
ip6gre_fill_info
Lorenzo Bianconi (2):
net: ip_gre: do not report erspan_ver for gre or gretap
net: ip6_gre: do not report erspan_ver for ip6gre or ip6gretap
net/ipv4/ip_gre.c | 33 +++++++++++++++++----------------
net/ipv6/ip6_gre.c | 36 ++++++++++++++++++------------------
2 files changed, 35 insertions(+), 34 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH net 1/2] net: ip_gre: do not report erspan_ver for gre or gretap
From: Lorenzo Bianconi @ 2019-02-19 16:42 UTC (permalink / raw)
To: davem; +Cc: netdev, u9012063
In-Reply-To: <cover.1550594081.git.lorenzo.bianconi@redhat.com>
Report erspan version field to userspace in ipgre_fill_info just for
erspan tunnels. The issue can be triggered with the following reproducer:
$ip link add name gre1 type gre local 192.168.0.1 remote 192.168.1.1
$ip link set dev gre1 up
$ip -d link sh gre1
13: gre1@NONE: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1476 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
link/gre 192.168.0.1 peer 192.168.1.1 promiscuity 0 minmtu 0 maxmtu 0
gre remote 192.168.1.1 local 192.168.0.1 ttl inherit erspan_ver 0 addrgenmode eui64 numtxqueues 1 numrxqueues 1
Fixes: f551c91de262 ("net: erspan: introduce erspan v2 for ip_gre")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
net/ipv4/ip_gre.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 3978f807fa8b..6ae89f2b541b 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1457,9 +1457,23 @@ static int ipgre_fill_info(struct sk_buff *skb, const struct net_device *dev)
struct ip_tunnel_parm *p = &t->parms;
__be16 o_flags = p->o_flags;
- if ((t->erspan_ver == 1 || t->erspan_ver == 2) &&
- !t->collect_md)
- o_flags |= TUNNEL_KEY;
+ if (t->erspan_ver == 1 || t->erspan_ver == 2) {
+ if (!t->collect_md)
+ o_flags |= TUNNEL_KEY;
+
+ if (nla_put_u8(skb, IFLA_GRE_ERSPAN_VER, t->erspan_ver))
+ goto nla_put_failure;
+
+ if (t->erspan_ver == 1) {
+ if (nla_put_u32(skb, IFLA_GRE_ERSPAN_INDEX, t->index))
+ goto nla_put_failure;
+ } else {
+ if (nla_put_u8(skb, IFLA_GRE_ERSPAN_DIR, t->dir))
+ goto nla_put_failure;
+ if (nla_put_u16(skb, IFLA_GRE_ERSPAN_HWID, t->hwid))
+ goto nla_put_failure;
+ }
+ }
if (nla_put_u32(skb, IFLA_GRE_LINK, p->link) ||
nla_put_be16(skb, IFLA_GRE_IFLAGS,
@@ -1495,19 +1509,6 @@ static int ipgre_fill_info(struct sk_buff *skb, const struct net_device *dev)
goto nla_put_failure;
}
- if (nla_put_u8(skb, IFLA_GRE_ERSPAN_VER, t->erspan_ver))
- goto nla_put_failure;
-
- if (t->erspan_ver == 1) {
- if (nla_put_u32(skb, IFLA_GRE_ERSPAN_INDEX, t->index))
- goto nla_put_failure;
- } else if (t->erspan_ver == 2) {
- if (nla_put_u8(skb, IFLA_GRE_ERSPAN_DIR, t->dir))
- goto nla_put_failure;
- if (nla_put_u16(skb, IFLA_GRE_ERSPAN_HWID, t->hwid))
- goto nla_put_failure;
- }
-
return 0;
nla_put_failure:
--
2.20.1
^ permalink raw reply related
* [PATCH net 2/2] net: ip6_gre: do not report erspan_ver for ip6gre or ip6gretap
From: Lorenzo Bianconi @ 2019-02-19 16:42 UTC (permalink / raw)
To: davem; +Cc: netdev, u9012063
In-Reply-To: <cover.1550594081.git.lorenzo.bianconi@redhat.com>
Report erspan version field to userspace in ip6gre_fill_info just for
erspan_v6 tunnels. Moreover report IFLA_GRE_ERSPAN_INDEX only for
erspan version 1.
The issue can be triggered with the following reproducer:
$ip link add name gre6 type ip6gre local 2001::1 remote 2002::2
$ip link set gre6 up
$ip -d link sh gre6
14: grep6@NONE: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1448 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
link/gre6 2001::1 peer 2002::2 promiscuity 0 minmtu 0 maxmtu 0
ip6gre remote 2002::2 local 2001::1 hoplimit 64 encaplimit 4 tclass 0x00 flowlabel 0x00000 erspan_index 0 erspan_ver 0 addrgenmode eui64
Fixes: 94d7d8f29287 ("ip6_gre: add erspan v2 support")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
net/ipv6/ip6_gre.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 43890898b0b5..0fdd0109d131 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -2104,9 +2104,23 @@ static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
struct __ip6_tnl_parm *p = &t->parms;
__be16 o_flags = p->o_flags;
- if ((p->erspan_ver == 1 || p->erspan_ver == 2) &&
- !p->collect_md)
- o_flags |= TUNNEL_KEY;
+ if (p->erspan_ver == 1 || p->erspan_ver == 2) {
+ if (!p->collect_md)
+ o_flags |= TUNNEL_KEY;
+
+ if (nla_put_u8(skb, IFLA_GRE_ERSPAN_VER, p->erspan_ver))
+ goto nla_put_failure;
+
+ if (p->erspan_ver == 1) {
+ if (nla_put_u32(skb, IFLA_GRE_ERSPAN_INDEX, p->index))
+ goto nla_put_failure;
+ } else {
+ if (nla_put_u8(skb, IFLA_GRE_ERSPAN_DIR, p->dir))
+ goto nla_put_failure;
+ if (nla_put_u16(skb, IFLA_GRE_ERSPAN_HWID, p->hwid))
+ goto nla_put_failure;
+ }
+ }
if (nla_put_u32(skb, IFLA_GRE_LINK, p->link) ||
nla_put_be16(skb, IFLA_GRE_IFLAGS,
@@ -2121,8 +2135,7 @@ static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
nla_put_u8(skb, IFLA_GRE_ENCAP_LIMIT, p->encap_limit) ||
nla_put_be32(skb, IFLA_GRE_FLOWINFO, p->flowinfo) ||
nla_put_u32(skb, IFLA_GRE_FLAGS, p->flags) ||
- nla_put_u32(skb, IFLA_GRE_FWMARK, p->fwmark) ||
- nla_put_u32(skb, IFLA_GRE_ERSPAN_INDEX, p->index))
+ nla_put_u32(skb, IFLA_GRE_FWMARK, p->fwmark))
goto nla_put_failure;
if (nla_put_u16(skb, IFLA_GRE_ENCAP_TYPE,
@@ -2140,19 +2153,6 @@ static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
goto nla_put_failure;
}
- if (nla_put_u8(skb, IFLA_GRE_ERSPAN_VER, p->erspan_ver))
- goto nla_put_failure;
-
- if (p->erspan_ver == 1) {
- if (nla_put_u32(skb, IFLA_GRE_ERSPAN_INDEX, p->index))
- goto nla_put_failure;
- } else if (p->erspan_ver == 2) {
- if (nla_put_u8(skb, IFLA_GRE_ERSPAN_DIR, p->dir))
- goto nla_put_failure;
- if (nla_put_u16(skb, IFLA_GRE_ERSPAN_HWID, p->hwid))
- goto nla_put_failure;
- }
-
return 0;
nla_put_failure:
--
2.20.1
^ permalink raw reply related
* Re: [PATCH RFC 2/5] net/sched: prepare TC actions to properly validate the control action
From: Davide Caratti @ 2019-02-19 16:50 UTC (permalink / raw)
To: Vlad Buslov
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Paolo Abeni, netdev@vger.kernel.org
In-Reply-To: <vbfbm39qbsw.fsf@mellanox.com>
hi Vlad,
On Mon, 2019-02-18 at 16:52 +0000, Vlad Buslov wrote:
> Hi Davide,
>
> I really like the idea of putting all action validation code in single
> place, instead of spreading it between act API and specific actions init
> code.
sure, the previous validation code was in tcf_action_add_1(), and I
attempted to fix the problems (well, it's the same problem showing up in
3 different ways) without adding the same code everywhere.
Sadly, we need to handle correctly the situations where
tcf_action_check_ctrlact() returns an error, and I'm seeing that we
can't fix every action with the same exact code everywhere (to avoid
leaking IDRs, or memory, or both).
> See my comment regarding minor problem with using
> tcf_action_set_ctrlact() on current net-next below.
> > +void tcf_action_set_ctrlact(struct tc_action *p, int action,
> > + struct tcf_chain *goto_chain)
> > +{
> > + struct tcf_chain *old;
> > +
> > + old = xchg(&p->goto_chain, goto_chain);
> > + if (old)
> > + tcf_chain_put_by_act(old);
>
> This is blocking in current net-next because tcf_chain_put_by_act()
> obtains block->lock mutex, so you can't call it while holding tcf_lock
> spinlock like you do in following patches. Its not a big problem but
> I guess you will have to just inline this code into all init handlers
> instead of tcf_action_set_ctrlact() function.
Argh! you are right. Thanks a lot for spotting this.
Ok, let's kill tcf_action_set_ctrlact() and swap 'newchain' with
a->goto_chain in each action init(), with a->tcfa_lock taken. I will
then call tcf_chain_put_by_act() in the init() handler, after the
spinlock is released.
> BTW is atomic xchg strictly necessary if you hold tcf_lock while
> re-assigning goto_chain pointer?
No, it's not necessary. The new value of goto_chain is used only when the
new value of tcfa_action is updated: so, if we just do the assignment of
goto_chain and tcfa_action together with the spinlock taken, at least we
are improving the
current code.
Thanks!
--
davide
^ permalink raw reply
* Re: [PATCH RFC 0/5] net/sched: validate the control action with all the other parameters
From: Davide Caratti @ 2019-02-19 16:51 UTC (permalink / raw)
To: Cong Wang
Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Vlad Buslov,
Paolo Abeni, Linux Kernel Network Developers
In-Reply-To: <CAM_iQpX5ogcHsX+9JFxj-C8XMQmHxVM+fV+rkvv9mBK2yL2zNQ@mail.gmail.com>
On Mon, 2019-02-18 at 22:42 -0800, Cong Wang wrote:
> On Fri, Feb 15, 2019 at 3:06 PM Davide Caratti <dcaratti@redhat.com> wrote:
> > currently, the kernel checks for bad values of the control action in
> > tcf_action_init_1(), after a successful call to the action's init()
> > function. This causes three bad behaviors:
>
> Yeah, I have been complaining about this for a long time,
> although slightly differently. The problem here is the lack of
> "copy" in RCU mechanism, which makes it nearly impossible
> to rollback to the previous state of an action on failure path
> of an update, which also makes RCU readers reading a partially
> updated action too.
thanks for looking at this code.
by the way, I see that act_mirred has an error path after the
assignment of tcfm_eaction and tcfa_action, and this is again causing
a fail in the 'replace with bad action' tests ('half write', issue #1).
Since it's the same problem, I will fix this in the same patch (moving the
assignment after the 'if' test on the value of parm->ifindex.
> Before I fix the "copy" part, your fixes make sense to me. There
> might be some other way to expose the action-specific tcfa_action
> opcode, but it might not be better than yours.
>
> BTW, please fold these bad behaviors into each appropriate
> patch, and keep the cover letter as an overview of the whole
> patchset rather than showing any details.
>
> [...]
Ok, and I plan to add a selftest for each action - so that it's possible
to verify functionality (at least problem #1) before and after each
ommit.
> > all these three problems can be fixed if we validate the control action
> > in the init() function, in the same way as we are already doing for all
> > the other parameters.
> >
> > - patch 1 is a temporary fix for problem 2), but it's reverted at the
> > end of the series
>
> Please drop patch 1, it is very unlikely only patch 1 will be backported,
> I think the whole patchset should be, therefore we have no reason
> to carry a temporary fix here.
sure, I will do that.
thanks!
--
davide
^ permalink raw reply
* Re: [PATCH 0/2] ARM: dts: am335x-evm/evmsk: Fix PHY mode for ethernet
From: Tony Lindgren @ 2019-02-19 16:52 UTC (permalink / raw)
To: Peter Ujfalusi
Cc: bcousson, linux-omap, devicetree, linux-arm-kernel, nsekhar,
grygorii.strashko, vkoul, netdev, f.fainelli, marc.w.gonzalez,
niklas.cassel
In-Reply-To: <c35030fb-9107-1919-5c18-1a39ace8c036@ti.com>
* Peter Ujfalusi <peter.ujfalusi@ti.com> [190219 08:02]:
> On 18/02/2019 18.26, Tony Lindgren wrote:
> > OK so sounds like these are OK to wait for v5.1 merge window
> > then as the dts changes alone won't fix anything?
>
> I think it would be better to send these to 5.0 to avoid the regression
> siting in linux-next.
>
> The patches are actually doing to the correct thing (we need rgmii-id,
> not rmgii-txid for these boards).
> It was just a matter of luck that it worked with rgmii-txid as the
> driver was broken and did not disabled the rxid when it should have.
OK applying into omap-for-v5.0/fixes-v2. I'll send a pull
request on Thursday most likely then, let's see if it makes
it for v5.0.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH bpf-next v2] bpf: bpftool, fix documentation for attach types
From: Daniel Borkmann @ 2019-02-19 16:24 UTC (permalink / raw)
To: Alban Crequy, ast, quentin.monnet
Cc: netdev, linux-kernel, john.fastabend, alban, iago
In-Reply-To: <20190219141332.23103-1-alban@kinvolk.io>
On 02/19/2019 03:13 PM, Alban Crequy wrote:
> From: Alban Crequy <alban@kinvolk.io>
>
> bpftool has support for attach types "stream_verdict" and
> "stream_parser" but the documentation was referring to them as
> "skb_verdict" and "skb_parse". The inconsistency comes from commit
> b7d3826c2ed6 ("bpf: bpftool, add support for attaching programs to
> maps").
>
> This patch changes the documentation to match the implementation:
> - "bpftool prog help"
> - man pages
> - bash completion
>
> Signed-off-by: Alban Crequy <alban@kinvolk.io>
Applied, thanks Alban!
^ 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