* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
From: Markus Elfring @ 2019-08-31 17:26 UTC (permalink / raw)
To: Denis Efremov, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong, David S. Miller,
Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß, Leon Romanovsky,
linux-arm-msm, linux-fsdevel, linux-input, linux-kernel,
linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
In-Reply-To: <c5e4479d-2fb3-b5a5-00c3-b06e5177d869@linux.com>
>>>>> +# nested likely/unlikely calls
>>>>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>>>> + WARN("LIKELY_MISUSE",
…
>> \b(?:un)?likely\s*
>
> This pair of brackets is required to match "unlikely"
> and it's optional in order to match "likely".
I agree also to this view if you refer to the shortened regular expression here.
But I got an other development opinion for an extra pair of non-capturing parentheses
at the front (from the version which you suggested).
Regards,
Markus
^ permalink raw reply
* Re: [PATCH] net: dsa: Fix off-by-one number of calls to devlink_port_unregister
From: Vivien Didelot @ 2019-08-31 17:17 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: f.fainelli, andrew, davem, netdev, Vladimir Oltean
In-Reply-To: <20190831124619.460-1-olteanv@gmail.com>
On Sat, 31 Aug 2019 15:46:19 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> When a function such as dsa_slave_create fails, currently the following
> stack trace can be seen:
>
> [ 2.038342] sja1105 spi0.1: Probed switch chip: SJA1105T
> [ 2.054556] sja1105 spi0.1: Reset switch and programmed static config
> [ 2.063837] sja1105 spi0.1: Enabled switch tagging
> [ 2.068706] fsl-gianfar soc:ethernet@2d90000 eth2: error -19 setting up slave phy
> [ 2.076371] ------------[ cut here ]------------
> [ 2.080973] WARNING: CPU: 1 PID: 21 at net/core/devlink.c:6184 devlink_free+0x1b4/0x1c0
> [ 2.088954] Modules linked in:
> [ 2.092005] CPU: 1 PID: 21 Comm: kworker/1:1 Not tainted 5.3.0-rc6-01360-g41b52e38d2b6-dirty #1746
> [ 2.100912] Hardware name: Freescale LS1021A
> [ 2.105162] Workqueue: events deferred_probe_work_func
> [ 2.110287] [<c03133a4>] (unwind_backtrace) from [<c030d8cc>] (show_stack+0x10/0x14)
> [ 2.117992] [<c030d8cc>] (show_stack) from [<c10b08d8>] (dump_stack+0xb4/0xc8)
> [ 2.125180] [<c10b08d8>] (dump_stack) from [<c0349d04>] (__warn+0xe0/0xf8)
> [ 2.132018] [<c0349d04>] (__warn) from [<c0349e34>] (warn_slowpath_null+0x40/0x48)
> [ 2.139549] [<c0349e34>] (warn_slowpath_null) from [<c0f19d74>] (devlink_free+0x1b4/0x1c0)
> [ 2.147772] [<c0f19d74>] (devlink_free) from [<c1064fc0>] (dsa_switch_teardown+0x60/0x6c)
> [ 2.155907] [<c1064fc0>] (dsa_switch_teardown) from [<c1065950>] (dsa_register_switch+0x8e4/0xaa8)
> [ 2.164821] [<c1065950>] (dsa_register_switch) from [<c0ba7fe4>] (sja1105_probe+0x21c/0x2ec)
> [ 2.173216] [<c0ba7fe4>] (sja1105_probe) from [<c0b35948>] (spi_drv_probe+0x80/0xa4)
> [ 2.180920] [<c0b35948>] (spi_drv_probe) from [<c0a4c1cc>] (really_probe+0x108/0x400)
> [ 2.188711] [<c0a4c1cc>] (really_probe) from [<c0a4c694>] (driver_probe_device+0x78/0x1bc)
> [ 2.196933] [<c0a4c694>] (driver_probe_device) from [<c0a4a3dc>] (bus_for_each_drv+0x58/0xb8)
> [ 2.205414] [<c0a4a3dc>] (bus_for_each_drv) from [<c0a4c024>] (__device_attach+0xd0/0x168)
> [ 2.213637] [<c0a4c024>] (__device_attach) from [<c0a4b1d0>] (bus_probe_device+0x84/0x8c)
> [ 2.221772] [<c0a4b1d0>] (bus_probe_device) from [<c0a4b72c>] (deferred_probe_work_func+0x84/0xc4)
> [ 2.230686] [<c0a4b72c>] (deferred_probe_work_func) from [<c03650a4>] (process_one_work+0x218/0x510)
> [ 2.239772] [<c03650a4>] (process_one_work) from [<c03660d8>] (worker_thread+0x2a8/0x5c0)
> [ 2.247908] [<c03660d8>] (worker_thread) from [<c036b348>] (kthread+0x148/0x150)
> [ 2.255265] [<c036b348>] (kthread) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
> [ 2.262444] Exception stack(0xea965fb0 to 0xea965ff8)
> [ 2.267466] 5fa0: 00000000 00000000 00000000 00000000
> [ 2.275598] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 2.283729] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [ 2.290333] ---[ end trace ca5d506728a0581a ]---
>
> devlink_free is complaining right here:
>
> WARN_ON(!list_empty(&devlink->port_list));
>
> This happens because devlink_port_unregister is no longer done right
> away in dsa_port_setup when a DSA_PORT_TYPE_USER has failed.
> Vivien said about this change that:
>
> Also no need to call devlink_port_unregister from within dsa_port_setup
> as this step is inconditionally handled by dsa_port_teardown on error.
>
> which is not really true. The devlink_port_unregister function _is_
> being called unconditionally from within dsa_port_setup, but not for
> this port that just failed, just for the previous ones which were set
> up.
>
> ports_teardown:
> for (i = 0; i < port; i++)
> dsa_port_teardown(&ds->ports[i]);
>
> Initially I was tempted to fix this by extending the "for" loop to also
> cover the port that failed during setup. But this could have potentially
> unforeseen consequences unrelated to devlink_port or even other types of
> ports than user ports, which I can't really test for. For example, if
> for some reason devlink_port_register itself would fail, then
> unconditionally unregistering it in dsa_port_teardown would not be a
> smart idea. The list might go on.
>
> So just make dsa_port_setup undo the setup it had done upon failure, and
> let the for loop undo the work of setting up the previous ports, which
> are guaranteed to be brought up to a consistent state.
>
> Fixes: 955222ca5281 ("net: dsa: use a single switch statement for port setup")
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>
This belongs to net-next. Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH] net: dsa: Fix off-by-one number of calls to devlink_port_unregister
From: Vivien Didelot @ 2019-08-31 17:14 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: Florian Fainelli, Andrew Lunn, David S. Miller, netdev
In-Reply-To: <CA+h21hoKcg3UUNkYRyEw8FS0q_vxdmoQL90BaOuKoW074DYYow@mail.gmail.com>
Hi Vladimir,
On Sat, 31 Aug 2019 19:54:40 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> Fine, I had not noticed the "registered" field from devlink_port.
> But I fail to see how dsa_port_teardown can be entered in the generic
> case from whatever failure state dsa_port_setup left it in. What if
> it's a DSA_PORT_TYPE_CPU whose devlink_port_register failed. What will
> happen to the PHYLINK instance behind dsa_port_link_register_of (not
> to mention about data that the driver might be allocating in
> dsa_port_enable and expecting a matching disable so it won't leak)?
> And that doesn't mean the fix isn't "proper". It may be "supposed" to
> be called unconditionally on error, but right now it isn't, so I doubt
> anybody has tested that, and that there aren't corner cases. Just
> playing the safe side.
You are correct, while I think PHYLINK handles disconnecting properly,
I'm not sure dsa_port_disable is ready yet. Your proposed fix is a
safer solution in the meantime.
> > BTW that is the subtlety between "unregister" which considers that the object
> > _may_ have been registered, and "deregister" which assumes the object _was_
>
> That concept is not familiar to me. Actually I grepped the DSA API for
> "unregister" and found:
That is not a kernel concept, I was simply pointing out the definitions
from the English language, as I found this interesting. Unfortunately
this isn't honored everywhere in the code, as you've noticed ;-)
> > registered. Would you like to go ahead and propose the devlink patch?
>
> Nope, I don't really know what I'm getting myself into :) If you want
> to send it, I will consider it during v2.
Sure, I'll propose it myself.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
From: Denis Efremov @ 2019-08-31 17:07 UTC (permalink / raw)
To: Markus Elfring, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong, David S. Miller,
Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß, Leon Romanovsky,
linux-arm-msm, linux-fsdevel, linux-input, linux-kernel,
linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
In-Reply-To: <493a7377-2de9-1d44-cd8f-c658793d15db@web.de>
On 31.08.2019 19:45, Markus Elfring wrote:
>>>> +# nested likely/unlikely calls
>>>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>>> + WARN("LIKELY_MISUSE",
>>>
>>> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>>> in this regular expression?
> …
>> IS_ERR
>> (?:_ <- Another atomic group just to show that '_' is a common prefix?
>
> Yes. - I hope that this specification detail can help a bit.
I'm not sure that another pair of brackets for a single char worth it.
>> Usually, Perl interpreter is very good at optimizing such things.
The interpreter optimizes it internally:
echo 'IS_ERR_OR_NULL' | perl -Mre=debug -ne '/IS_ERR(?:_OR_NULL|_VALUE)?/ && print'
Compiling REx "IS_ERR(?:_OR_NULL|_VALUE)?"
Final program:
1: EXACT <IS_ERR> (4)
4: CURLYX[0]{0,1} (16)
6: EXACT <_> (8) <-- common prefix
8: TRIE-EXACT[OV] (15)
<OR_NULL>
<VALUE>
...
>
> Would you like to help this software component by omitting a pair of
> non-capturing parentheses at the beginning?
>
> \b(?:un)?likely\s*
This pair of brackets is required to match "unlikely" and it's
optional in order to match "likely".
Regards,
Denis
^ permalink raw reply
* Re: [RFC PATCH v2 net-next 00/15] tc-taprio offload for SJA1105 DSA
From: Vladimir Oltean @ 2019-08-31 17:03 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Florian Fainelli, Vivien Didelot, Andrew Lunn, David S. Miller,
Vinicius Costa Gomes, vedang.patel, Richard Cochran, weifeng.voon,
jiri, m-karicheri2, Jose.Abreu, Ilias Apalodimas,
Jamal Hadi Salim, xiyou.wangcong, netdev
In-Reply-To: <20190830152839.0fe34d25@cakuba.netronome.com>
On Sat, 31 Aug 2019 at 01:29, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 30 Aug 2019 13:11:11 +0300, Vladimir Oltean wrote:
> > On Fri, 30 Aug 2019 at 04:21, Jakub Kicinski wrote:
> > > On Fri, 30 Aug 2019 03:46:20 +0300, Vladimir Oltean wrote:
> > > > - Configuring the switch over SPI cannot apparently be done from this
> > > > ndo_setup_tc callback because it runs in atomic context. I also have
> > > > some downstream patches to offload tc clsact matchall with mirred
> > > > action, but in that case it looks like the atomic context restriction
> > > > does not apply.
> > >
> > > This sounds really surprising ndo_setup_tc should always be allowed to
> > > sleep. Can the taprio limitation be lifted somehow?
> >
> > I need to get more familiar with the taprio internal data structures.
> > I think you're suggesting to get those updated to a consistent state
> > while under spin_lock_bh(qdisc_lock(sch)), then call ndo_setup_tc from
> > outside that critical section?
>
> I'm not 100% sure how taprio handles locking TBH, it just seems naive
> that HW callback will not need to sleep, so the kernel should make sure
> that callback can sleep. Otherwise we'll end up with 3/4 of drivers
> implementing some async work routine...
>
> Sorry, I know that's quite general and not that helpful.
>
> > Also, I just noticed that I introduced a bug in taprio_disable_offload
> > with my reference counting addition. The qdisc can't just pass stack
> > memory to the driver, now that it's allowing it to keep it. So
> > definitely the patch needs more refactoring.
>
> Ah, a slight deja vu, I think someone else has done it in the past :)
Ok, I think I managed to move the ndo_setup_tc callback outside of
atomic context and nothing broke so far... Any other comments or
should I go ahead and send a first proper patchset?
Thanks,
-Vladimir
^ permalink raw reply
* Re: [PATCH] net: dsa: Fix off-by-one number of calls to devlink_port_unregister
From: Vladimir Oltean @ 2019-08-31 16:54 UTC (permalink / raw)
To: Vivien Didelot; +Cc: Florian Fainelli, Andrew Lunn, David S. Miller, netdev
In-Reply-To: <20190831121958.GC12031@t480s.localdomain>
Hi Vivien,
On Sat, 31 Aug 2019 at 19:20, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> Hi Vladimir,
>
> On Sat, 31 Aug 2019 15:46:19 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > When a function such as dsa_slave_create fails, currently the following
> > stack trace can be seen:
> >
> > [ 2.038342] sja1105 spi0.1: Probed switch chip: SJA1105T
> > [ 2.054556] sja1105 spi0.1: Reset switch and programmed static config
> > [ 2.063837] sja1105 spi0.1: Enabled switch tagging
> > [ 2.068706] fsl-gianfar soc:ethernet@2d90000 eth2: error -19 setting up slave phy
> > [ 2.076371] ------------[ cut here ]------------
> > [ 2.080973] WARNING: CPU: 1 PID: 21 at net/core/devlink.c:6184 devlink_free+0x1b4/0x1c0
> > [ 2.088954] Modules linked in:
> > [ 2.092005] CPU: 1 PID: 21 Comm: kworker/1:1 Not tainted 5.3.0-rc6-01360-g41b52e38d2b6-dirty #1746
> > [ 2.100912] Hardware name: Freescale LS1021A
> > [ 2.105162] Workqueue: events deferred_probe_work_func
> > [ 2.110287] [<c03133a4>] (unwind_backtrace) from [<c030d8cc>] (show_stack+0x10/0x14)
> > [ 2.117992] [<c030d8cc>] (show_stack) from [<c10b08d8>] (dump_stack+0xb4/0xc8)
> > [ 2.125180] [<c10b08d8>] (dump_stack) from [<c0349d04>] (__warn+0xe0/0xf8)
> > [ 2.132018] [<c0349d04>] (__warn) from [<c0349e34>] (warn_slowpath_null+0x40/0x48)
> > [ 2.139549] [<c0349e34>] (warn_slowpath_null) from [<c0f19d74>] (devlink_free+0x1b4/0x1c0)
> > [ 2.147772] [<c0f19d74>] (devlink_free) from [<c1064fc0>] (dsa_switch_teardown+0x60/0x6c)
> > [ 2.155907] [<c1064fc0>] (dsa_switch_teardown) from [<c1065950>] (dsa_register_switch+0x8e4/0xaa8)
> > [ 2.164821] [<c1065950>] (dsa_register_switch) from [<c0ba7fe4>] (sja1105_probe+0x21c/0x2ec)
> > [ 2.173216] [<c0ba7fe4>] (sja1105_probe) from [<c0b35948>] (spi_drv_probe+0x80/0xa4)
> > [ 2.180920] [<c0b35948>] (spi_drv_probe) from [<c0a4c1cc>] (really_probe+0x108/0x400)
> > [ 2.188711] [<c0a4c1cc>] (really_probe) from [<c0a4c694>] (driver_probe_device+0x78/0x1bc)
> > [ 2.196933] [<c0a4c694>] (driver_probe_device) from [<c0a4a3dc>] (bus_for_each_drv+0x58/0xb8)
> > [ 2.205414] [<c0a4a3dc>] (bus_for_each_drv) from [<c0a4c024>] (__device_attach+0xd0/0x168)
> > [ 2.213637] [<c0a4c024>] (__device_attach) from [<c0a4b1d0>] (bus_probe_device+0x84/0x8c)
> > [ 2.221772] [<c0a4b1d0>] (bus_probe_device) from [<c0a4b72c>] (deferred_probe_work_func+0x84/0xc4)
> > [ 2.230686] [<c0a4b72c>] (deferred_probe_work_func) from [<c03650a4>] (process_one_work+0x218/0x510)
> > [ 2.239772] [<c03650a4>] (process_one_work) from [<c03660d8>] (worker_thread+0x2a8/0x5c0)
> > [ 2.247908] [<c03660d8>] (worker_thread) from [<c036b348>] (kthread+0x148/0x150)
> > [ 2.255265] [<c036b348>] (kthread) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
> > [ 2.262444] Exception stack(0xea965fb0 to 0xea965ff8)
> > [ 2.267466] 5fa0: 00000000 00000000 00000000 00000000
> > [ 2.275598] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> > [ 2.283729] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > [ 2.290333] ---[ end trace ca5d506728a0581a ]---
> >
> > devlink_free is complaining right here:
> >
> > WARN_ON(!list_empty(&devlink->port_list));
> >
> > This happens because devlink_port_unregister is no longer done right
> > away in dsa_port_setup when a DSA_PORT_TYPE_USER has failed.
> > Vivien said about this change that:
> >
> > Also no need to call devlink_port_unregister from within dsa_port_setup
> > as this step is inconditionally handled by dsa_port_teardown on error.
> >
> > which is not really true. The devlink_port_unregister function _is_
> > being called unconditionally from within dsa_port_setup, but not for
>
> Not from within dsa_port_setup, but from its caller dsa_tree_setup_switches.
>
> > this port that just failed, just for the previous ones which were set
> > up.
> >
> > ports_teardown:
> > for (i = 0; i < port; i++)
> > dsa_port_teardown(&ds->ports[i]);
> >
> > Initially I was tempted to fix this by extending the "for" loop to also
> > cover the port that failed during setup. But this could have potentially
> > unforeseen consequences unrelated to devlink_port or even other types of
> > ports than user ports, which I can't really test for. For example, if
> > for some reason devlink_port_register itself would fail, then
> > unconditionally unregistering it in dsa_port_teardown would not be a
> > smart idea. The list might go on.
> >
> > So just make dsa_port_setup undo the setup it had done upon failure, and
> > let the for loop undo the work of setting up the previous ports, which
> > are guaranteed to be brought up to a consistent state.
> >
> > Fixes: 955222ca5281 ("net: dsa: use a single switch statement for port setup")
> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> > ---
> > net/dsa/dsa2.c | 39 +++++++++++++++++++++++++++++----------
> > 1 file changed, 29 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> > index f8445fa73448..b501c90aabe4 100644
> > --- a/net/dsa/dsa2.c
> > +++ b/net/dsa/dsa2.c
> > @@ -259,8 +259,11 @@ static int dsa_port_setup(struct dsa_port *dp)
> > const unsigned char *id = (const unsigned char *)&dst->index;
> > const unsigned char len = sizeof(dst->index);
> > struct devlink_port *dlp = &dp->devlink_port;
> > + bool dsa_port_link_registered = false;
> > + bool devlink_port_registered = false;
> > struct devlink *dl = ds->devlink;
> > - int err;
> > + bool dsa_port_enabled = false;
> > + int err = 0;
> >
> > switch (dp->type) {
> > case DSA_PORT_TYPE_UNUSED:
> > @@ -272,15 +275,19 @@ static int dsa_port_setup(struct dsa_port *dp)
> > dp->index, false, 0, id, len);
> > err = devlink_port_register(dl, dlp, dp->index);
> > if (err)
> > - return err;
> > + break;
> > + devlink_port_registered = true;
> >
> > err = dsa_port_link_register_of(dp);
> > if (err)
> > - return err;
> > + break;
> > + dsa_port_link_registered = true;
> >
> > err = dsa_port_enable(dp, NULL);
> > if (err)
> > - return err;
> > + break;
> > + dsa_port_enabled = true;
> > +
> > break;
> > case DSA_PORT_TYPE_DSA:
> > memset(dlp, 0, sizeof(*dlp));
> > @@ -288,15 +295,19 @@ static int dsa_port_setup(struct dsa_port *dp)
> > dp->index, false, 0, id, len);
> > err = devlink_port_register(dl, dlp, dp->index);
> > if (err)
> > - return err;
> > + break;
> > + devlink_port_registered = true;
> >
> > err = dsa_port_link_register_of(dp);
> > if (err)
> > - return err;
> > + break;
> > + dsa_port_link_registered = true;
> >
> > err = dsa_port_enable(dp, NULL);
> > if (err)
> > - return err;
> > + break;
> > + dsa_port_enabled = true;
> > +
> > break;
> > case DSA_PORT_TYPE_USER:
> > memset(dlp, 0, sizeof(*dlp));
> > @@ -304,18 +315,26 @@ static int dsa_port_setup(struct dsa_port *dp)
> > dp->index, false, 0, id, len);
> > err = devlink_port_register(dl, dlp, dp->index);
> > if (err)
> > - return err;
> > + break;
> > + devlink_port_registered = true;
> >
> > dp->mac = of_get_mac_address(dp->dn);
> > err = dsa_slave_create(dp);
> > if (err)
> > - return err;
> > + break;
> >
> > devlink_port_type_eth_set(dlp, dp->slave);
> > break;
> > }
> >
> > - return 0;
> > + if (err && dsa_port_enabled)
> > + dsa_port_disable(dp);
> > + if (err && dsa_port_link_registered)
> > + dsa_port_link_unregister_of(dp);
> > + if (err && devlink_port_registered)
> > + devlink_port_unregister(dlp);
> > +
> > + return err;
> > }
>
> No no, I'm pretty sure you can tell this is going to be a nightmare to
> maintain these boolean states for all port types ;-)
>
> And this is not a proper fix for the problem you've spotted. The problem
> you've spotted is that devlink_port_unregister isn't called for the current
> port if its setup failed, because dsa_port_teardown -- which is supposed to
> be called unconditionally on error -- isn't called for the current port. Your
> first attempt was correct, simply fix the loop in dsa_tree_setup_switches
> to include the current port:
>
Fine, I had not noticed the "registered" field from devlink_port.
But I fail to see how dsa_port_teardown can be entered in the generic
case from whatever failure state dsa_port_setup left it in. What if
it's a DSA_PORT_TYPE_CPU whose devlink_port_register failed. What will
happen to the PHYLINK instance behind dsa_port_link_register_of (not
to mention about data that the driver might be allocating in
dsa_port_enable and expecting a matching disable so it won't leak)?
And that doesn't mean the fix isn't "proper". It may be "supposed" to
be called unconditionally on error, but right now it isn't, so I doubt
anybody has tested that, and that there aren't corner cases. Just
playing the safe side.
>
> ports_teardown:
> - for (i = 0; i < port; i++)
> + for (i = 0; i <= port; i++)
>
>
> As for devlink_port_unregister, most kernel APIs unregistering objects are
> self protected, so I'm tempted to propose the following patch for devlink:
>
>
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 650f36379203..ab95607800d6 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -6264,6 +6264,8 @@ void devlink_port_unregister(struct devlink_port *devlink_port)
> {
> struct devlink *devlink = devlink_port->devlink;
>
> + if (!devlink_port->registered)
> + return;
> devlink_port_type_warn_cancel(devlink_port);
> devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_DEL);
> mutex_lock(&devlink->lock);
>
>
> Otherwise we can protect the devlink port unregistering ourselves with:
>
>
> if (dlp->registered)
> devlink_port_unregister(dlp);
>
>
> BTW that is the subtlety between "unregister" which considers that the object
> _may_ have been registered, and "deregister" which assumes the object _was_
That concept is not familiar to me. Actually I grepped the DSA API for
"unregister" and found:
static void dsa_tag_driver_unregister(struct dsa_tag_driver *dsa_tag_driver)
{
mutex_lock(&dsa_tag_drivers_lock);
list_del(&dsa_tag_driver->list);
mutex_unlock(&dsa_tag_drivers_lock);
}
which looks pretty unconditional to me?
> registered. Would you like to go ahead and propose the devlink patch?
>
Nope, I don't really know what I'm getting myself into :) If you want
to send it, I will consider it during v2.
>
> Thanks for pointing this out,
>
> Vivien
Regards,
-Vladimir
^ permalink raw reply
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
From: Markus Elfring @ 2019-08-31 16:45 UTC (permalink / raw)
To: Denis Efremov, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong, David S. Miller,
Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß, Leon Romanovsky,
linux-arm-msm, linux-fsdevel, linux-input, linux-kernel,
linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
In-Reply-To: <689c8baf-2298-f086-3461-5cd1cdd191c6@linux.com>
>>> +# nested likely/unlikely calls
>>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>> + WARN("LIKELY_MISUSE",
>>
>> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>> in this regular expression?
…
> IS_ERR
> (?:_ <- Another atomic group just to show that '_' is a common prefix?
Yes. - I hope that this specification detail can help a bit.
> Usually, Perl interpreter is very good at optimizing such things.
Would you like to help this software component by omitting a pair of
non-capturing parentheses at the beginning?
\b(?:un)?likely\s*
Regards,
Markus
^ permalink raw reply
* Re: [PATCH net] rxrpc: Fix lack of conn cleanup when local endpoint is cleaned up [ver #2]
From: David Howells @ 2019-08-31 16:45 UTC (permalink / raw)
To: Hillf Danton; +Cc: dhowells, netdev, marc.dionne, linux-afs, linux-kernel
In-Reply-To: <20190831135906.6028-1-hdanton@sina.com>
Hillf Danton <hdanton@sina.com> wrote:
> > - if (rxnet->live) {
> > + if (rxnet->live && !conn->params.local->dead) {
> > idle_timestamp = READ_ONCE(conn->idle_timestamp);
> > expire_at = idle_timestamp + rxrpc_connection_expiry * HZ;
> > if (conn->params.local->service_closed)
>
> Is there any chance out there that this reaper starts running one minute
> after the dead local went into graveyard?
It's certainly possible that that can happen. The reaper is per
network-namespace.
conn->params.local holds a ref on the local endpoint.
It may be worth wrapping the "local->dead = true;" in rxrpc_local_destroyer()
in rxnet->conn_lock.
David
^ permalink raw reply
* Re: [PATCH] net: dsa: Fix off-by-one number of calls to devlink_port_unregister
From: Vivien Didelot @ 2019-08-31 16:19 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: f.fainelli, andrew, davem, netdev, Vladimir Oltean
In-Reply-To: <20190831124619.460-1-olteanv@gmail.com>
Hi Vladimir,
On Sat, 31 Aug 2019 15:46:19 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> When a function such as dsa_slave_create fails, currently the following
> stack trace can be seen:
>
> [ 2.038342] sja1105 spi0.1: Probed switch chip: SJA1105T
> [ 2.054556] sja1105 spi0.1: Reset switch and programmed static config
> [ 2.063837] sja1105 spi0.1: Enabled switch tagging
> [ 2.068706] fsl-gianfar soc:ethernet@2d90000 eth2: error -19 setting up slave phy
> [ 2.076371] ------------[ cut here ]------------
> [ 2.080973] WARNING: CPU: 1 PID: 21 at net/core/devlink.c:6184 devlink_free+0x1b4/0x1c0
> [ 2.088954] Modules linked in:
> [ 2.092005] CPU: 1 PID: 21 Comm: kworker/1:1 Not tainted 5.3.0-rc6-01360-g41b52e38d2b6-dirty #1746
> [ 2.100912] Hardware name: Freescale LS1021A
> [ 2.105162] Workqueue: events deferred_probe_work_func
> [ 2.110287] [<c03133a4>] (unwind_backtrace) from [<c030d8cc>] (show_stack+0x10/0x14)
> [ 2.117992] [<c030d8cc>] (show_stack) from [<c10b08d8>] (dump_stack+0xb4/0xc8)
> [ 2.125180] [<c10b08d8>] (dump_stack) from [<c0349d04>] (__warn+0xe0/0xf8)
> [ 2.132018] [<c0349d04>] (__warn) from [<c0349e34>] (warn_slowpath_null+0x40/0x48)
> [ 2.139549] [<c0349e34>] (warn_slowpath_null) from [<c0f19d74>] (devlink_free+0x1b4/0x1c0)
> [ 2.147772] [<c0f19d74>] (devlink_free) from [<c1064fc0>] (dsa_switch_teardown+0x60/0x6c)
> [ 2.155907] [<c1064fc0>] (dsa_switch_teardown) from [<c1065950>] (dsa_register_switch+0x8e4/0xaa8)
> [ 2.164821] [<c1065950>] (dsa_register_switch) from [<c0ba7fe4>] (sja1105_probe+0x21c/0x2ec)
> [ 2.173216] [<c0ba7fe4>] (sja1105_probe) from [<c0b35948>] (spi_drv_probe+0x80/0xa4)
> [ 2.180920] [<c0b35948>] (spi_drv_probe) from [<c0a4c1cc>] (really_probe+0x108/0x400)
> [ 2.188711] [<c0a4c1cc>] (really_probe) from [<c0a4c694>] (driver_probe_device+0x78/0x1bc)
> [ 2.196933] [<c0a4c694>] (driver_probe_device) from [<c0a4a3dc>] (bus_for_each_drv+0x58/0xb8)
> [ 2.205414] [<c0a4a3dc>] (bus_for_each_drv) from [<c0a4c024>] (__device_attach+0xd0/0x168)
> [ 2.213637] [<c0a4c024>] (__device_attach) from [<c0a4b1d0>] (bus_probe_device+0x84/0x8c)
> [ 2.221772] [<c0a4b1d0>] (bus_probe_device) from [<c0a4b72c>] (deferred_probe_work_func+0x84/0xc4)
> [ 2.230686] [<c0a4b72c>] (deferred_probe_work_func) from [<c03650a4>] (process_one_work+0x218/0x510)
> [ 2.239772] [<c03650a4>] (process_one_work) from [<c03660d8>] (worker_thread+0x2a8/0x5c0)
> [ 2.247908] [<c03660d8>] (worker_thread) from [<c036b348>] (kthread+0x148/0x150)
> [ 2.255265] [<c036b348>] (kthread) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
> [ 2.262444] Exception stack(0xea965fb0 to 0xea965ff8)
> [ 2.267466] 5fa0: 00000000 00000000 00000000 00000000
> [ 2.275598] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 2.283729] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [ 2.290333] ---[ end trace ca5d506728a0581a ]---
>
> devlink_free is complaining right here:
>
> WARN_ON(!list_empty(&devlink->port_list));
>
> This happens because devlink_port_unregister is no longer done right
> away in dsa_port_setup when a DSA_PORT_TYPE_USER has failed.
> Vivien said about this change that:
>
> Also no need to call devlink_port_unregister from within dsa_port_setup
> as this step is inconditionally handled by dsa_port_teardown on error.
>
> which is not really true. The devlink_port_unregister function _is_
> being called unconditionally from within dsa_port_setup, but not for
Not from within dsa_port_setup, but from its caller dsa_tree_setup_switches.
> this port that just failed, just for the previous ones which were set
> up.
>
> ports_teardown:
> for (i = 0; i < port; i++)
> dsa_port_teardown(&ds->ports[i]);
>
> Initially I was tempted to fix this by extending the "for" loop to also
> cover the port that failed during setup. But this could have potentially
> unforeseen consequences unrelated to devlink_port or even other types of
> ports than user ports, which I can't really test for. For example, if
> for some reason devlink_port_register itself would fail, then
> unconditionally unregistering it in dsa_port_teardown would not be a
> smart idea. The list might go on.
>
> So just make dsa_port_setup undo the setup it had done upon failure, and
> let the for loop undo the work of setting up the previous ports, which
> are guaranteed to be brought up to a consistent state.
>
> Fixes: 955222ca5281 ("net: dsa: use a single switch statement for port setup")
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
> net/dsa/dsa2.c | 39 +++++++++++++++++++++++++++++----------
> 1 file changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index f8445fa73448..b501c90aabe4 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -259,8 +259,11 @@ static int dsa_port_setup(struct dsa_port *dp)
> const unsigned char *id = (const unsigned char *)&dst->index;
> const unsigned char len = sizeof(dst->index);
> struct devlink_port *dlp = &dp->devlink_port;
> + bool dsa_port_link_registered = false;
> + bool devlink_port_registered = false;
> struct devlink *dl = ds->devlink;
> - int err;
> + bool dsa_port_enabled = false;
> + int err = 0;
>
> switch (dp->type) {
> case DSA_PORT_TYPE_UNUSED:
> @@ -272,15 +275,19 @@ static int dsa_port_setup(struct dsa_port *dp)
> dp->index, false, 0, id, len);
> err = devlink_port_register(dl, dlp, dp->index);
> if (err)
> - return err;
> + break;
> + devlink_port_registered = true;
>
> err = dsa_port_link_register_of(dp);
> if (err)
> - return err;
> + break;
> + dsa_port_link_registered = true;
>
> err = dsa_port_enable(dp, NULL);
> if (err)
> - return err;
> + break;
> + dsa_port_enabled = true;
> +
> break;
> case DSA_PORT_TYPE_DSA:
> memset(dlp, 0, sizeof(*dlp));
> @@ -288,15 +295,19 @@ static int dsa_port_setup(struct dsa_port *dp)
> dp->index, false, 0, id, len);
> err = devlink_port_register(dl, dlp, dp->index);
> if (err)
> - return err;
> + break;
> + devlink_port_registered = true;
>
> err = dsa_port_link_register_of(dp);
> if (err)
> - return err;
> + break;
> + dsa_port_link_registered = true;
>
> err = dsa_port_enable(dp, NULL);
> if (err)
> - return err;
> + break;
> + dsa_port_enabled = true;
> +
> break;
> case DSA_PORT_TYPE_USER:
> memset(dlp, 0, sizeof(*dlp));
> @@ -304,18 +315,26 @@ static int dsa_port_setup(struct dsa_port *dp)
> dp->index, false, 0, id, len);
> err = devlink_port_register(dl, dlp, dp->index);
> if (err)
> - return err;
> + break;
> + devlink_port_registered = true;
>
> dp->mac = of_get_mac_address(dp->dn);
> err = dsa_slave_create(dp);
> if (err)
> - return err;
> + break;
>
> devlink_port_type_eth_set(dlp, dp->slave);
> break;
> }
>
> - return 0;
> + if (err && dsa_port_enabled)
> + dsa_port_disable(dp);
> + if (err && dsa_port_link_registered)
> + dsa_port_link_unregister_of(dp);
> + if (err && devlink_port_registered)
> + devlink_port_unregister(dlp);
> +
> + return err;
> }
No no, I'm pretty sure you can tell this is going to be a nightmare to
maintain these boolean states for all port types ;-)
And this is not a proper fix for the problem you've spotted. The problem
you've spotted is that devlink_port_unregister isn't called for the current
port if its setup failed, because dsa_port_teardown -- which is supposed to
be called unconditionally on error -- isn't called for the current port. Your
first attempt was correct, simply fix the loop in dsa_tree_setup_switches
to include the current port:
ports_teardown:
- for (i = 0; i < port; i++)
+ for (i = 0; i <= port; i++)
As for devlink_port_unregister, most kernel APIs unregistering objects are
self protected, so I'm tempted to propose the following patch for devlink:
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 650f36379203..ab95607800d6 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6264,6 +6264,8 @@ void devlink_port_unregister(struct devlink_port *devlink_port)
{
struct devlink *devlink = devlink_port->devlink;
+ if (!devlink_port->registered)
+ return;
devlink_port_type_warn_cancel(devlink_port);
devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_DEL);
mutex_lock(&devlink->lock);
Otherwise we can protect the devlink port unregistering ourselves with:
if (dlp->registered)
devlink_port_unregister(dlp);
BTW that is the subtlety between "unregister" which considers that the object
_may_ have been registered, and "deregister" which assumes the object _was_
registered. Would you like to go ahead and propose the devlink patch?
Thanks for pointing this out,
Vivien
^ permalink raw reply
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
From: Denis Efremov @ 2019-08-31 15:54 UTC (permalink / raw)
To: Markus Elfring, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong, David S. Miller,
Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß, Leon Romanovsky,
linux-arm-msm, linux-fsdevel, linux-input, linux-kernel,
linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
In-Reply-To: <0d9345ed-f16a-de0b-6125-1f663765eb46@web.de>
On 31.08.2019 12:15, Markus Elfring wrote:
>> +# nested likely/unlikely calls
>> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>> + WARN("LIKELY_MISUSE",
>
> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
> in this regular expression?
Hmm,
(?: <- Catch group is required here, since it is used in diagnostic message,
see $1
IS_ERR
(?:_ <- Another atomic group just to show that '_' is a common prefix?
I'm not sure about this. Usually, Perl interpreter is very good at optimizing such things.
You could see this optimization if you run perl with -Mre=debug.
(?:OR_NULL|VALUE))?|WARN)
Regards,
Denis
^ permalink raw reply
* Re: iproute2: tc: potential buffer overflow
From: Stephen Hemminger @ 2019-08-31 15:37 UTC (permalink / raw)
To: tomaspaukrt; +Cc: netdev
In-Reply-To: <8fo.ZWfD.3kvedbSyU2M.1TQd9t@seznam.cz>
On Sat, 31 Aug 2019 15:13:27 +0200 (CEST)
<tomaspaukrt@email.cz> wrote:
> Hi,
>
> there are two potentially dangerous calls of strcpy function in the program "tc". In the attachment is a patch that fixes this issue.
>
> Tomas
This looks correct.
Please fix with strlcpy() instead; that is clearer.
Plus you can use XT_EXTENSION_MAX_NAMELEN here (optional).
^ permalink raw reply
* Re: [PATCH v2 2/2] PTP: add support for one-shot output
From: Richard Cochran @ 2019-08-31 15:01 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel, davem
In-Reply-To: <87r253ulpn.fsf@gmail.com>
On Fri, Aug 30, 2019 at 11:00:20AM +0300, Felipe Balbi wrote:
> seems like this should be defined together with the other flags? If
> that's the case, it seems like we would EXTTS and PEROUT masks.
Yes, let's make the meanings of the bit fields clear...
--- ptp_clock.h ---
/*
* Bits of the ptp_extts_request.flags field:
*/
#define PTP_ENABLE_FEATURE BIT(0)
#define PTP_RISING_EDGE BIT(1)
#define PTP_FALLING_EDGE BIT(2)
#define PTP_EXTTS_VALID_FLAGS (PTP_ENABLE_FEATURE | \
PTP_RISING_EDGE | \
PTP_FALLING_EDGE)
/*
* Bits of the ptp_perout_request.flags field:
*/
#define PTP_PEROUT_ONE_SHOT BIT(0)
#define PTP_PEROUT_VALID_FLAGS (PTP_PEROUT_ONE_SHOT)
struct ptp_extts_request {
unsigned int flags; /* Bit field of PTP_EXTTS_VALID_FLAGS. */
};
struct ptp_perout_request {
unsigned int flags; /* Bit field of PTP_PEROUT_VALID_FLAGS. */
};
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH v2 2/2] PTP: add support for one-shot output
From: Richard Cochran @ 2019-08-31 14:47 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Christopher S Hall, netdev, linux-kernel, davem
In-Reply-To: <87r253ulpn.fsf@gmail.com>
On Fri, Aug 30, 2019 at 11:00:20AM +0300, Felipe Balbi wrote:
> >> @@ -177,9 +177,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> >> err = -EFAULT;
> >> break;
> >> }
> >> - if ((req.perout.flags || req.perout.rsv[0] || req.perout.rsv[1]
> >> - || req.perout.rsv[2] || req.perout.rsv[3])
> >> - && cmd == PTP_PEROUT_REQUEST2) {
> >> + if ((req.perout.rsv[0] || req.perout.rsv[1] || req.perout.rsv[2]
> >> + || req.perout.rsv[3]) && cmd == PTP_PEROUT_REQUEST2) {
> >
> > Please check that the reserved bits of req.perout.flags, namely
> > ~PTP_PEROUT_ONE_SHOT, are clear.
>
> Actually, we should check more. PEROUT_FEATURE_ENABLE is still valid
> here, right? So are RISING and FALLING edges, no?
No. The ptp_extts_request.flags are indeed defined:
struct ptp_extts_request {
...
unsigned int flags; /* Bit field for PTP_xxx flags. */
...
};
But the ptp_perout_request.flags are reserved:
struct ptp_perout_request {
...
unsigned int flags; /* Reserved for future use. */
...
};
For this ioctl, the test for enable/disable is
ptp_perout_request.period is zero:
enable = req.perout.period.sec || req.perout.period.nsec;
err = ops->enable(ops, &req, enable);
The usage pattern here is taken from timer_settime(2).
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH 0/4 net-next] flow_offload: update mangle action representation
From: Pablo Neira Ayuso @ 2019-08-31 14:22 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netfilter-devel, davem, netdev, vishal, saeedm, jiri
In-Reply-To: <20190830153351.5d5330fa@cakuba.netronome.com>
On Fri, Aug 30, 2019 at 03:33:51PM -0700, Jakub Kicinski wrote:
> On Fri, 30 Aug 2019 11:07:10 +0200, Pablo Neira Ayuso wrote:
> > > > * The front-end coalesces consecutive pedit actions into one single
> > > > word, so drivers can mangle IPv6 and ethernet address fields in one
> > > > single go.
> > >
> > > You still only coalesce up to 16 bytes, no?
> >
> > You only have to rise FLOW_ACTION_MANGLE_MAXLEN coming in this patch
> > if you need more. I don't know of any packet field larger than 16
> > bytes. If there is a use-case for this, it should be easy to rise that
> > definition.
>
> Please see the definitions of:
>
> struct nfp_fl_set_eth
> struct nfp_fl_set_ip4_addrs
> struct nfp_fl_set_ip4_ttl_tos
> struct nfp_fl_set_ipv6_tc_hl_fl
> struct nfp_fl_set_ipv6_addr
> struct nfp_fl_set_tport
>
> These are the programming primitives for header rewrites in the NFP.
> Since each of those contains more than just one field, we'll have to
> keep all the field coalescing logic in the driver, even if you coalesce
> while fields (i.e. IPv6 addresses).
nfp has been updated in this patch series to deal with the new mangle
representation.
> Perhaps it's not a serious blocker for the series, but it'd be nice if
> rewrite action grouping was handled in the core. Since you're already
> poking at that code..
Rewrite action grouping is already handled from the core front-end in
this patch series.
Thanks.
^ permalink raw reply
* Re: [PATCH 4.14] tcp: fix tcp_rtx_queue_tail in case of empty retransmit queue
From: Matthieu Baerts @ 2019-08-31 13:14 UTC (permalink / raw)
To: Sasha Levin, Tim Froidcoeur
Cc: aprout, cpaasch, davem, edumazet, gregkh, jonathan.lemon, jtl,
linux-kernel, mkubecek, ncardwell, stable, ycheng, netdev
In-Reply-To: <20190831122036.GY5281@sasha-vm>
Hi Sasha,
Thank you for your reply!
On 31/08/2019 14:20, Sasha Levin wrote:
> On Sat, Aug 24, 2019 at 08:03:51AM +0200, Tim Froidcoeur wrote:
>> Commit 8c3088f895a0 ("tcp: be more careful in tcp_fragment()")
>> triggers following stack trace:
>>
>> [25244.848046] kernel BUG at ./include/linux/skbuff.h:1406!
>> [25244.859335] RIP: 0010:skb_queue_prev+0x9/0xc
>> [25244.888167] Call Trace:
>> [25244.889182] <IRQ>
>> [25244.890001] tcp_fragment+0x9c/0x2cf
>> [25244.891295] tcp_write_xmit+0x68f/0x988
>> [25244.892732] __tcp_push_pending_frames+0x3b/0xa0
>> [25244.894347] tcp_data_snd_check+0x2a/0xc8
>> [25244.895775] tcp_rcv_established+0x2a8/0x30d
>> [25244.897282] tcp_v4_do_rcv+0xb2/0x158
>> [25244.898666] tcp_v4_rcv+0x692/0x956
>> [25244.899959] ip_local_deliver_finish+0xeb/0x169
>> [25244.901547] __netif_receive_skb_core+0x51c/0x582
>> [25244.903193] ? inet_gro_receive+0x239/0x247
>> [25244.904756] netif_receive_skb_internal+0xab/0xc6
>> [25244.906395] napi_gro_receive+0x8a/0xc0
>> [25244.907760] receive_buf+0x9a1/0x9cd
>> [25244.909160] ? load_balance+0x17a/0x7b7
>> [25244.910536] ? vring_unmap_one+0x18/0x61
>> [25244.911932] ? detach_buf+0x60/0xfa
>> [25244.913234] virtnet_poll+0x128/0x1e1
>> [25244.914607] net_rx_action+0x12a/0x2b1
>> [25244.915953] __do_softirq+0x11c/0x26b
>> [25244.917269] ? handle_irq_event+0x44/0x56
>> [25244.918695] irq_exit+0x61/0xa0
>> [25244.919947] do_IRQ+0x9d/0xbb
>> [25244.921065] common_interrupt+0x85/0x85
>> [25244.922479] </IRQ>
>>
>> tcp_rtx_queue_tail() (called by tcp_fragment()) can call
>> tcp_write_queue_prev() on the first packet in the queue, which will
>> trigger
>> the BUG in tcp_write_queue_prev(), because there is no previous packet.
>>
>> This happens when the retransmit queue is empty, for example in case of a
>> zero window.
>>
>> Patch is needed for 4.4, 4.9 and 4.14 stable branches.
>
> There needs to be a better explanation of why it's not needed
> upstream...
Commit 8c3088f895a0 ("tcp: be more careful in tcp_fragment()") was not a
simple cherry-pick of the original one from master (b617158dc096)
because there is a specific TCP rtx queue only since v4.15. For more
details, please see the commit message of b617158dc096 ("tcp: be more
careful in tcp_fragment()").
The BUG() is hit due to the specific code added to versions older than
v4.15. The comment in skb_queue_prev() (include/linux/skbuff.h:1406),
just before the BUG_ON() somehow suggests to add a check before using
it, what Tim did.
In master, this code path causing the issue will not be taken because
the implementation of tcp_rtx_queue_tail() is different:
tcp_fragment() → tcp_rtx_queue_tail() → tcp_write_queue_prev() →
skb_queue_prev() → BUG_ON()
Because this patch is specific to versions older than the two last
stable ones but still linked to the network architecture, who can review
and approve it? :)
Cheers,
Matt
--
Matthieu Baerts | R&D Engineer
matthieu.baerts@tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
^ permalink raw reply
* iproute2: tc: potential buffer overflow
From: tomaspaukrt @ 2019-08-31 13:13 UTC (permalink / raw)
To: netdev
[-- Attachment #1: Type: text/plain, Size: 149 bytes --]
Hi,
there are two potentially dangerous calls of strcpy function in the program "tc". In the attachment is a patch that fixes this issue.
Tomas
[-- Attachment #2: iproute2-overflow-fix.patch --]
[-- Type: text/x-diff, Size: 945 bytes --]
diff --git a/tc/m_ipt.c b/tc/m_ipt.c
index cc95eab7..cb64380b 100644
--- a/tc/m_ipt.c
+++ b/tc/m_ipt.c
@@ -269,7 +269,8 @@ static int build_st(struct xtables_target *target, struct ipt_entry_target *t)
} else {
target->t = t;
}
- strcpy(target->t->u.user.name, target->name);
+ strncpy(target->t->u.user.name, target->name,
+ sizeof(target->t->u.user.name) - 1);
return 0;
}
diff --git a/tc/m_xt_old.c b/tc/m_xt_old.c
index 6a4509a9..974ac496 100644
--- a/tc/m_xt_old.c
+++ b/tc/m_xt_old.c
@@ -177,7 +177,8 @@ build_st(struct xtables_target *target, struct xt_entry_target *t)
if (t == NULL) {
target->t = fw_calloc(1, size);
target->t->u.target_size = size;
- strcpy(target->t->u.user.name, target->name);
+ strncpy(target->t->u.user.name, target->name,
+ sizeof(target->t->u.user.name) - 1);
set_revision(target->t->u.user.name, target->revision);
if (target->init != NULL)
^ permalink raw reply related
* [PATCH] net: dsa: Fix off-by-one number of calls to devlink_port_unregister
From: Vladimir Oltean @ 2019-08-31 12:46 UTC (permalink / raw)
To: f.fainelli, vivien.didelot, andrew, davem; +Cc: netdev, Vladimir Oltean
When a function such as dsa_slave_create fails, currently the following
stack trace can be seen:
[ 2.038342] sja1105 spi0.1: Probed switch chip: SJA1105T
[ 2.054556] sja1105 spi0.1: Reset switch and programmed static config
[ 2.063837] sja1105 spi0.1: Enabled switch tagging
[ 2.068706] fsl-gianfar soc:ethernet@2d90000 eth2: error -19 setting up slave phy
[ 2.076371] ------------[ cut here ]------------
[ 2.080973] WARNING: CPU: 1 PID: 21 at net/core/devlink.c:6184 devlink_free+0x1b4/0x1c0
[ 2.088954] Modules linked in:
[ 2.092005] CPU: 1 PID: 21 Comm: kworker/1:1 Not tainted 5.3.0-rc6-01360-g41b52e38d2b6-dirty #1746
[ 2.100912] Hardware name: Freescale LS1021A
[ 2.105162] Workqueue: events deferred_probe_work_func
[ 2.110287] [<c03133a4>] (unwind_backtrace) from [<c030d8cc>] (show_stack+0x10/0x14)
[ 2.117992] [<c030d8cc>] (show_stack) from [<c10b08d8>] (dump_stack+0xb4/0xc8)
[ 2.125180] [<c10b08d8>] (dump_stack) from [<c0349d04>] (__warn+0xe0/0xf8)
[ 2.132018] [<c0349d04>] (__warn) from [<c0349e34>] (warn_slowpath_null+0x40/0x48)
[ 2.139549] [<c0349e34>] (warn_slowpath_null) from [<c0f19d74>] (devlink_free+0x1b4/0x1c0)
[ 2.147772] [<c0f19d74>] (devlink_free) from [<c1064fc0>] (dsa_switch_teardown+0x60/0x6c)
[ 2.155907] [<c1064fc0>] (dsa_switch_teardown) from [<c1065950>] (dsa_register_switch+0x8e4/0xaa8)
[ 2.164821] [<c1065950>] (dsa_register_switch) from [<c0ba7fe4>] (sja1105_probe+0x21c/0x2ec)
[ 2.173216] [<c0ba7fe4>] (sja1105_probe) from [<c0b35948>] (spi_drv_probe+0x80/0xa4)
[ 2.180920] [<c0b35948>] (spi_drv_probe) from [<c0a4c1cc>] (really_probe+0x108/0x400)
[ 2.188711] [<c0a4c1cc>] (really_probe) from [<c0a4c694>] (driver_probe_device+0x78/0x1bc)
[ 2.196933] [<c0a4c694>] (driver_probe_device) from [<c0a4a3dc>] (bus_for_each_drv+0x58/0xb8)
[ 2.205414] [<c0a4a3dc>] (bus_for_each_drv) from [<c0a4c024>] (__device_attach+0xd0/0x168)
[ 2.213637] [<c0a4c024>] (__device_attach) from [<c0a4b1d0>] (bus_probe_device+0x84/0x8c)
[ 2.221772] [<c0a4b1d0>] (bus_probe_device) from [<c0a4b72c>] (deferred_probe_work_func+0x84/0xc4)
[ 2.230686] [<c0a4b72c>] (deferred_probe_work_func) from [<c03650a4>] (process_one_work+0x218/0x510)
[ 2.239772] [<c03650a4>] (process_one_work) from [<c03660d8>] (worker_thread+0x2a8/0x5c0)
[ 2.247908] [<c03660d8>] (worker_thread) from [<c036b348>] (kthread+0x148/0x150)
[ 2.255265] [<c036b348>] (kthread) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
[ 2.262444] Exception stack(0xea965fb0 to 0xea965ff8)
[ 2.267466] 5fa0: 00000000 00000000 00000000 00000000
[ 2.275598] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 2.283729] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 2.290333] ---[ end trace ca5d506728a0581a ]---
devlink_free is complaining right here:
WARN_ON(!list_empty(&devlink->port_list));
This happens because devlink_port_unregister is no longer done right
away in dsa_port_setup when a DSA_PORT_TYPE_USER has failed.
Vivien said about this change that:
Also no need to call devlink_port_unregister from within dsa_port_setup
as this step is inconditionally handled by dsa_port_teardown on error.
which is not really true. The devlink_port_unregister function _is_
being called unconditionally from within dsa_port_setup, but not for
this port that just failed, just for the previous ones which were set
up.
ports_teardown:
for (i = 0; i < port; i++)
dsa_port_teardown(&ds->ports[i]);
Initially I was tempted to fix this by extending the "for" loop to also
cover the port that failed during setup. But this could have potentially
unforeseen consequences unrelated to devlink_port or even other types of
ports than user ports, which I can't really test for. For example, if
for some reason devlink_port_register itself would fail, then
unconditionally unregistering it in dsa_port_teardown would not be a
smart idea. The list might go on.
So just make dsa_port_setup undo the setup it had done upon failure, and
let the for loop undo the work of setting up the previous ports, which
are guaranteed to be brought up to a consistent state.
Fixes: 955222ca5281 ("net: dsa: use a single switch statement for port setup")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
net/dsa/dsa2.c | 39 +++++++++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index f8445fa73448..b501c90aabe4 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -259,8 +259,11 @@ static int dsa_port_setup(struct dsa_port *dp)
const unsigned char *id = (const unsigned char *)&dst->index;
const unsigned char len = sizeof(dst->index);
struct devlink_port *dlp = &dp->devlink_port;
+ bool dsa_port_link_registered = false;
+ bool devlink_port_registered = false;
struct devlink *dl = ds->devlink;
- int err;
+ bool dsa_port_enabled = false;
+ int err = 0;
switch (dp->type) {
case DSA_PORT_TYPE_UNUSED:
@@ -272,15 +275,19 @@ static int dsa_port_setup(struct dsa_port *dp)
dp->index, false, 0, id, len);
err = devlink_port_register(dl, dlp, dp->index);
if (err)
- return err;
+ break;
+ devlink_port_registered = true;
err = dsa_port_link_register_of(dp);
if (err)
- return err;
+ break;
+ dsa_port_link_registered = true;
err = dsa_port_enable(dp, NULL);
if (err)
- return err;
+ break;
+ dsa_port_enabled = true;
+
break;
case DSA_PORT_TYPE_DSA:
memset(dlp, 0, sizeof(*dlp));
@@ -288,15 +295,19 @@ static int dsa_port_setup(struct dsa_port *dp)
dp->index, false, 0, id, len);
err = devlink_port_register(dl, dlp, dp->index);
if (err)
- return err;
+ break;
+ devlink_port_registered = true;
err = dsa_port_link_register_of(dp);
if (err)
- return err;
+ break;
+ dsa_port_link_registered = true;
err = dsa_port_enable(dp, NULL);
if (err)
- return err;
+ break;
+ dsa_port_enabled = true;
+
break;
case DSA_PORT_TYPE_USER:
memset(dlp, 0, sizeof(*dlp));
@@ -304,18 +315,26 @@ static int dsa_port_setup(struct dsa_port *dp)
dp->index, false, 0, id, len);
err = devlink_port_register(dl, dlp, dp->index);
if (err)
- return err;
+ break;
+ devlink_port_registered = true;
dp->mac = of_get_mac_address(dp->dn);
err = dsa_slave_create(dp);
if (err)
- return err;
+ break;
devlink_port_type_eth_set(dlp, dp->slave);
break;
}
- return 0;
+ if (err && dsa_port_enabled)
+ dsa_port_disable(dp);
+ if (err && dsa_port_link_registered)
+ dsa_port_link_unregister_of(dp);
+ if (err && devlink_port_registered)
+ devlink_port_unregister(dlp);
+
+ return err;
}
static void dsa_port_teardown(struct dsa_port *dp)
--
2.17.1
^ permalink raw reply related
* [PATCH net-next] net: hns3: remove set but not used variable 'qos'
From: YueHaibing @ 2019-08-31 12:29 UTC (permalink / raw)
To: Yisen Zhuang, David S . Miller, Salil Mehta, Yunsheng Lin,
Zhongzhu Liu, Guangbin Huang, Peng Li, Huazhong Tan, Jian Shen
Cc: YueHaibing, netdev, kernel-janitors, Hulk Robot
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c: In function 'hclge_restore_vlan_table':
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c:8016:18: warning:
variable 'qos' set but not used [-Wunused-but-set-variable]
Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 70a214903da9 ("net: hns3: reduce the parameters of some functions")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index ce4b2280a8b0..2b65f2799846 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -8013,7 +8013,7 @@ static void hclge_restore_vlan_table(struct hnae3_handle *handle)
struct hclge_vport *vport = hclge_get_vport(handle);
struct hclge_vport_vlan_cfg *vlan, *tmp;
struct hclge_dev *hdev = vport->back;
- u16 vlan_proto, qos;
+ u16 vlan_proto;
u16 state, vlan_id;
int i;
@@ -8022,7 +8022,6 @@ static void hclge_restore_vlan_table(struct hnae3_handle *handle)
vport = &hdev->vport[i];
vlan_proto = vport->port_base_vlan_cfg.vlan_info.vlan_proto;
vlan_id = vport->port_base_vlan_cfg.vlan_info.vlan_tag;
- qos = vport->port_base_vlan_cfg.vlan_info.qos;
state = vport->port_base_vlan_cfg.state;
if (state != HNAE3_PORT_BASE_VLAN_DISABLE) {
^ permalink raw reply related
* [PATCH v2 net] net: Properly update v4 routes with v6 nexthop
From: Donald Sharp @ 2019-08-31 12:22 UTC (permalink / raw)
To: netdev, dsahern, sworley
When creating a v4 route that uses a v6 nexthop from a nexthop group.
Allow the kernel to properly send the nexthop as v6 via the RTA_VIA
attribute.
Broken behavior:
$ ip nexthop add via fe80::9 dev eth0
$ ip nexthop show
id 1 via fe80::9 dev eth0 scope link
$ ip route add 4.5.6.7/32 nhid 1
$ ip route show
default via 10.0.2.2 dev eth0
4.5.6.7 nhid 1 via 254.128.0.0 dev eth0
10.0.2.0/24 dev eth0 proto kernel scope link src 10.0.2.15
$
Fixed behavior:
$ ip nexthop add via fe80::9 dev eth0
$ ip nexthop show
id 1 via fe80::9 dev eth0 scope link
$ ip route add 4.5.6.7/32 nhid 1
$ ip route show
default via 10.0.2.2 dev eth0
4.5.6.7 nhid 1 via inet6 fe80::9 dev eth0
10.0.2.0/24 dev eth0 proto kernel scope link src 10.0.2.15
$
v2: Addresses code review comments from David Ahern
Fixes: dcb1ecb50edf (“ipv4: Prepare for fib6_nh from a nexthop object”)
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
---
include/net/ip_fib.h | 4 ++--
include/net/nexthop.h | 5 +++--
net/ipv4/fib_semantics.c | 23 +++++++++++++----------
net/ipv6/route.c | 11 ++++++-----
4 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index 4c81846ccce8..ab1ca9e238d2 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -513,7 +513,7 @@ int ip_valid_fib_dump_req(struct net *net, const struct nlmsghdr *nlh,
struct netlink_callback *cb);
int fib_nexthop_info(struct sk_buff *skb, const struct fib_nh_common *nh,
- unsigned char *flags, bool skip_oif);
+ u8 rt_family, unsigned char *flags, bool skip_oif);
int fib_add_nexthop(struct sk_buff *skb, const struct fib_nh_common *nh,
- int nh_weight);
+ int nh_weight, u8 rt_family);
#endif /* _NET_FIB_H */
diff --git a/include/net/nexthop.h b/include/net/nexthop.h
index 95f766c31c90..331ebbc94fe7 100644
--- a/include/net/nexthop.h
+++ b/include/net/nexthop.h
@@ -161,7 +161,8 @@ struct nexthop *nexthop_mpath_select(const struct nexthop *nh, int nhsel)
}
static inline
-int nexthop_mpath_fill_node(struct sk_buff *skb, struct nexthop *nh)
+int nexthop_mpath_fill_node(struct sk_buff *skb, struct nexthop *nh,
+ u8 rt_family)
{
struct nh_group *nhg = rtnl_dereference(nh->nh_grp);
int i;
@@ -172,7 +173,7 @@ int nexthop_mpath_fill_node(struct sk_buff *skb, struct nexthop *nh)
struct fib_nh_common *nhc = &nhi->fib_nhc;
int weight = nhg->nh_entries[i].weight;
- if (fib_add_nexthop(skb, nhc, weight) < 0)
+ if (fib_add_nexthop(skb, nhc, weight, rt_family) < 0)
return -EMSGSIZE;
}
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 2db089e10ba0..673e5f2a6108 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1582,7 +1582,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
}
int fib_nexthop_info(struct sk_buff *skb, const struct fib_nh_common *nhc,
- unsigned char *flags, bool skip_oif)
+ u8 rt_family, unsigned char *flags, bool skip_oif)
{
if (nhc->nhc_flags & RTNH_F_DEAD)
*flags |= RTNH_F_DEAD;
@@ -1613,7 +1613,7 @@ int fib_nexthop_info(struct sk_buff *skb, const struct fib_nh_common *nhc,
/* if gateway family does not match nexthop family
* gateway is encoded as RTA_VIA
*/
- if (nhc->nhc_gw_family != nhc->nhc_family) {
+ if (rt_family != nhc->nhc_gw_family) {
int alen = sizeof(struct in6_addr);
struct nlattr *nla;
struct rtvia *via;
@@ -1654,7 +1654,7 @@ EXPORT_SYMBOL_GPL(fib_nexthop_info);
#if IS_ENABLED(CONFIG_IP_ROUTE_MULTIPATH) || IS_ENABLED(CONFIG_IPV6)
int fib_add_nexthop(struct sk_buff *skb, const struct fib_nh_common *nhc,
- int nh_weight)
+ int nh_weight, u8 rt_family)
{
const struct net_device *dev = nhc->nhc_dev;
struct rtnexthop *rtnh;
@@ -1667,7 +1667,7 @@ int fib_add_nexthop(struct sk_buff *skb, const struct fib_nh_common *nhc,
rtnh->rtnh_hops = nh_weight - 1;
rtnh->rtnh_ifindex = dev ? dev->ifindex : 0;
- if (fib_nexthop_info(skb, nhc, &flags, true) < 0)
+ if (fib_nexthop_info(skb, nhc, rt_family, &flags, true) < 0)
goto nla_put_failure;
rtnh->rtnh_flags = flags;
@@ -1684,7 +1684,8 @@ EXPORT_SYMBOL_GPL(fib_add_nexthop);
#endif
#ifdef CONFIG_IP_ROUTE_MULTIPATH
-static int fib_add_multipath(struct sk_buff *skb, struct fib_info *fi)
+static int fib_add_multipath(struct sk_buff *skb, struct fib_info *fi,
+ u8 rt_family)
{
struct nlattr *mp;
@@ -1693,13 +1694,14 @@ static int fib_add_multipath(struct sk_buff *skb, struct fib_info *fi)
goto nla_put_failure;
if (unlikely(fi->nh)) {
- if (nexthop_mpath_fill_node(skb, fi->nh) < 0)
+ if (nexthop_mpath_fill_node(skb, fi->nh, rt_family) < 0)
goto nla_put_failure;
goto mp_end;
}
for_nexthops(fi) {
- if (fib_add_nexthop(skb, &nh->nh_common, nh->fib_nh_weight) < 0)
+ if (fib_add_nexthop(skb, &nh->nh_common, nh->fib_nh_weight,
+ rt_family) < 0)
goto nla_put_failure;
#ifdef CONFIG_IP_ROUTE_CLASSID
if (nh->nh_tclassid &&
@@ -1717,7 +1719,8 @@ static int fib_add_multipath(struct sk_buff *skb, struct fib_info *fi)
return -EMSGSIZE;
}
#else
-static int fib_add_multipath(struct sk_buff *skb, struct fib_info *fi)
+static int fib_add_multipath(struct sk_buff *skb, struct fib_info *fi,
+ u8 family)
{
return 0;
}
@@ -1775,7 +1778,7 @@ int fib_dump_info(struct sk_buff *skb, u32 portid, u32 seq, int event,
const struct fib_nh_common *nhc = fib_info_nhc(fi, 0);
unsigned char flags = 0;
- if (fib_nexthop_info(skb, nhc, &flags, false) < 0)
+ if (fib_nexthop_info(skb, nhc, AF_INET, &flags, false) < 0)
goto nla_put_failure;
rtm->rtm_flags = flags;
@@ -1790,7 +1793,7 @@ int fib_dump_info(struct sk_buff *skb, u32 portid, u32 seq, int event,
}
#endif
} else {
- if (fib_add_multipath(skb, fi) < 0)
+ if (fib_add_multipath(skb, fi, AF_INET) < 0)
goto nla_put_failure;
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index fd059e08785a..cfb969e68d45 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -5329,7 +5329,7 @@ static int rt6_fill_node_nexthop(struct sk_buff *skb, struct nexthop *nh,
if (!mp)
goto nla_put_failure;
- if (nexthop_mpath_fill_node(skb, nh))
+ if (nexthop_mpath_fill_node(skb, nh, AF_INET6))
goto nla_put_failure;
nla_nest_end(skb, mp);
@@ -5337,7 +5337,7 @@ static int rt6_fill_node_nexthop(struct sk_buff *skb, struct nexthop *nh,
struct fib6_nh *fib6_nh;
fib6_nh = nexthop_fib6_nh(nh);
- if (fib_nexthop_info(skb, &fib6_nh->nh_common,
+ if (fib_nexthop_info(skb, &fib6_nh->nh_common, AF_INET6,
flags, false) < 0)
goto nla_put_failure;
}
@@ -5466,13 +5466,14 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
goto nla_put_failure;
if (fib_add_nexthop(skb, &rt->fib6_nh->nh_common,
- rt->fib6_nh->fib_nh_weight) < 0)
+ rt->fib6_nh->fib_nh_weight, AF_INET6) < 0)
goto nla_put_failure;
list_for_each_entry_safe(sibling, next_sibling,
&rt->fib6_siblings, fib6_siblings) {
if (fib_add_nexthop(skb, &sibling->fib6_nh->nh_common,
- sibling->fib6_nh->fib_nh_weight) < 0)
+ sibling->fib6_nh->fib_nh_weight,
+ AF_INET6) < 0)
goto nla_put_failure;
}
@@ -5489,7 +5490,7 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
rtm->rtm_flags |= nh_flags;
} else {
- if (fib_nexthop_info(skb, &rt->fib6_nh->nh_common,
+ if (fib_nexthop_info(skb, &rt->fib6_nh->nh_common, AF_INET6,
&nh_flags, false) < 0)
goto nla_put_failure;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 4.14] tcp: fix tcp_rtx_queue_tail in case of empty retransmit queue
From: Sasha Levin @ 2019-08-31 12:20 UTC (permalink / raw)
To: Tim Froidcoeur
Cc: matthieu.baerts, aprout, cpaasch, davem, edumazet, gregkh,
jonathan.lemon, jtl, linux-kernel, mkubecek, ncardwell, stable,
ycheng, netdev
In-Reply-To: <20190824060351.3776-1-tim.froidcoeur@tessares.net>
On Sat, Aug 24, 2019 at 08:03:51AM +0200, Tim Froidcoeur wrote:
>Commit 8c3088f895a0 ("tcp: be more careful in tcp_fragment()")
>triggers following stack trace:
>
>[25244.848046] kernel BUG at ./include/linux/skbuff.h:1406!
>[25244.859335] RIP: 0010:skb_queue_prev+0x9/0xc
>[25244.888167] Call Trace:
>[25244.889182] <IRQ>
>[25244.890001] tcp_fragment+0x9c/0x2cf
>[25244.891295] tcp_write_xmit+0x68f/0x988
>[25244.892732] __tcp_push_pending_frames+0x3b/0xa0
>[25244.894347] tcp_data_snd_check+0x2a/0xc8
>[25244.895775] tcp_rcv_established+0x2a8/0x30d
>[25244.897282] tcp_v4_do_rcv+0xb2/0x158
>[25244.898666] tcp_v4_rcv+0x692/0x956
>[25244.899959] ip_local_deliver_finish+0xeb/0x169
>[25244.901547] __netif_receive_skb_core+0x51c/0x582
>[25244.903193] ? inet_gro_receive+0x239/0x247
>[25244.904756] netif_receive_skb_internal+0xab/0xc6
>[25244.906395] napi_gro_receive+0x8a/0xc0
>[25244.907760] receive_buf+0x9a1/0x9cd
>[25244.909160] ? load_balance+0x17a/0x7b7
>[25244.910536] ? vring_unmap_one+0x18/0x61
>[25244.911932] ? detach_buf+0x60/0xfa
>[25244.913234] virtnet_poll+0x128/0x1e1
>[25244.914607] net_rx_action+0x12a/0x2b1
>[25244.915953] __do_softirq+0x11c/0x26b
>[25244.917269] ? handle_irq_event+0x44/0x56
>[25244.918695] irq_exit+0x61/0xa0
>[25244.919947] do_IRQ+0x9d/0xbb
>[25244.921065] common_interrupt+0x85/0x85
>[25244.922479] </IRQ>
>
>tcp_rtx_queue_tail() (called by tcp_fragment()) can call
>tcp_write_queue_prev() on the first packet in the queue, which will trigger
>the BUG in tcp_write_queue_prev(), because there is no previous packet.
>
>This happens when the retransmit queue is empty, for example in case of a
>zero window.
>
>Patch is needed for 4.4, 4.9 and 4.14 stable branches.
There needs to be a better explanation of why it's not needed
upstream...
--
Thanks,
Sasha
^ permalink raw reply
* RE: [PATCH 4.14] tcp: fix tcp_rtx_queue_tail in case of empty retransmit queue
From: maowenan @ 2019-08-31 11:44 UTC (permalink / raw)
To: David Miller, cpaasch@apple.com
Cc: jonathan.lemon@gmail.com, stable@vger.kernel.org,
gregkh@linuxfoundation.org, tim.froidcoeur@tessares.net,
matthieu.baerts@tessares.net, aprout@ll.mit.edu,
edumazet@google.com, jtl@netflix.com,
linux-kernel@vger.kernel.org, mkubecek@suse.cz,
ncardwell@google.com, sashal@kernel.org, ycheng@google.com,
netdev@vger.kernel.org
In-Reply-To: <20190830.192049.1447010488040109227.davem@davemloft.net>
Tim
Can you share the reproduce steps for this issue? C or syzkaller is ok.
Thanks a lot.
^ permalink raw reply
* Recall: [PATCH 4.14] tcp: fix tcp_rtx_queue_tail in case of empty retransmit queue
From: maowenan @ 2019-08-31 10:54 UTC (permalink / raw)
To: David Miller, cpaasch@apple.com
Cc: jonathan.lemon@gmail.com, stable@vger.kernel.org,
gregkh@linuxfoundation.org, tim.froidcoeur@tessares.net,
matthieu.baerts@tessares.net, aprout@ll.mit.edu,
edumazet@google.com, jtl@netflix.com,
linux-kernel@vger.kernel.org, mkubecek@suse.cz,
ncardwell@google.com, sashal@kernel.org, ycheng@google.com,
netdev@vger.kernel.org
maowenan would like to recall the message, "[PATCH 4.14] tcp: fix tcp_rtx_queue_tail in case of empty retransmit queue".
^ permalink raw reply
* RE: [PATCH 4.14] tcp: fix tcp_rtx_queue_tail in case of empty retransmit queue
From: maowenan @ 2019-08-31 10:53 UTC (permalink / raw)
To: David Miller, cpaasch@apple.com
Cc: jonathan.lemon@gmail.com, stable@vger.kernel.org,
gregkh@linuxfoundation.org, tim.froidcoeur@tessares.net,
matthieu.baerts@tessares.net, aprout@ll.mit.edu,
edumazet@google.com, jtl@netflix.com,
linux-kernel@vger.kernel.org, mkubecek@suse.cz,
ncardwell@google.com, sashal@kernel.org, ycheng@google.com,
netdev@vger.kernel.org
In-Reply-To: <20190830.192049.1447010488040109227.davem@davemloft.net>
[<ffffff90094930dc>] skb_peek_tail include/linux/skbuff.h:1516 [inline]
[<ffffff90094930dc>] tcp_write_queue_tail include/net/tcp.h:1478 [inline]
[<ffffff90094930dc>] tcp_rtx_queue_tail include/net/tcp.h:1543 [inline]
[<ffffff90094930dc>] tcp_fragment+0xc64/0xce8 net/ipv4/tcp_output.c:1175
[<ffffff90094a37f0>] tcp_write_wakeup+0x3f8/0x4a0 net/ipv4/tcp_output.c:3496
[<ffffff90094a38d0>] tcp_send_probe0+0x38/0x2d8 net/ipv4/tcp_output.c:3523
[<ffffff90094a75a0>] tcp_probe_timer net/ipv4/tcp_timer.c:343 [inline]
[<ffffff90094a75a0>] tcp_write_timer_handler+0x640/0x720 net/ipv4/tcp_timer.c:548
[<ffffff90094a76f8>] tcp_write_timer+0x78/0x1d0 net/ipv4/tcp_timer.c:562
[<ffffff90082610b0>] call_timer_fn.isra.1+0x58/0x180 kernel/time/timer.c:1144
[<ffffff90082616ec>] __run_timers kernel/time/timer.c:1218 [inline]
[<ffffff90082616ec>] run_timer_softirq+0x514/0x848 kernel/time/timer.c:1401
[<ffffff9008141a28>] __do_softirq+0x340/0x878 kernel/softirq.c:273
[<ffffff9008142890>] do_softirq_own_stack include/linux/interrupt.h:498 [inline]
[<ffffff9008142890>] invoke_softirq kernel/softirq.c:357 [inline]
[<ffffff9008142890>] irq_exit+0x170/0x370 kernel/softirq.c:391
[<ffffff900821d550>] __handle_domain_irq+0x100/0x1c0 kernel/irq/irqdesc.c:459
[<ffffff90080914a0>] handle_domain_irq include/linux/irqdesc.h:168 [inline]
[<ffffff90080914a0>] gic_handle_irq+0xd0/0x1f0 drivers/irqchip/irq-gic.c:377
^ permalink raw reply
* Re: [PATCH][next] zd1211rw: zd_usb: Use struct_size() helper
From: kbuild test robot @ 2019-08-31 9:38 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: kbuild-all, Daniel Drake, Ulrich Kunitz, Kalle Valo,
David S. Miller, linux-wireless, netdev, linux-kernel,
Gustavo A. R. Silva
In-Reply-To: <20190830185716.GA10044@embeddedor>
[-- Attachment #1: Type: text/plain, Size: 7287 bytes --]
Hi "Gustavo,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc6 next-20190830]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Gustavo-A-R-Silva/zd1211rw-zd_usb-Use-struct_size-helper/20190831-161121
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/net/wireless/zydas/zd1211rw/zd_usb.c:22:0:
drivers/net/wireless/zydas/zd1211rw/zd_usb.c: In function 'check_read_regs':
>> drivers/net/wireless/zydas/zd1211rw/zd_def.h:18:25: warning: format '%ld' expects argument of type 'long int', but argument 6 has type 'size_t {aka unsigned int}' [-Wformat=]
dev_printk(level, dev, "%s() " fmt, __func__, ##args)
^
>> drivers/net/wireless/zydas/zd1211rw/zd_def.h:22:4: note: in expansion of macro 'dev_printk_f'
dev_printk_f(KERN_DEBUG, dev, fmt, ## args)
^~~~~~~~~~~~
>> drivers/net/wireless/zydas/zd1211rw/zd_usb.c:1635:3: note: in expansion of macro 'dev_dbg_f'
dev_dbg_f(zd_usb_dev(usb),
^~~~~~~~~
drivers/net/wireless/zydas/zd1211rw/zd_usb.c:1636:51: note: format string is defined here
"error: actual length %d less than expected %ld\n",
~~^
%d
vim +/dev_dbg_f +1635 drivers/net/wireless/zydas/zd1211rw/zd_usb.c
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1622
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1623 static bool check_read_regs(struct zd_usb *usb, struct usb_req_read_regs *req,
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1624 unsigned int count)
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1625 {
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1626 int i;
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1627 struct zd_usb_interrupt *intr = &usb->intr;
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1628 struct read_regs_int *rr = &intr->read_regs;
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1629 struct usb_int_regs *regs = (struct usb_int_regs *)rr->buffer;
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1630
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1631 /* The created block size seems to be larger than expected.
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1632 * However results appear to be correct.
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1633 */
d352eeccec7e84 drivers/net/wireless/zydas/zd1211rw/zd_usb.c Gustavo A. R. Silva 2019-08-30 1634 if (rr->length < struct_size(regs, regs, count)) {
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 @1635 dev_dbg_f(zd_usb_dev(usb),
d352eeccec7e84 drivers/net/wireless/zydas/zd1211rw/zd_usb.c Gustavo A. R. Silva 2019-08-30 1636 "error: actual length %d less than expected %ld\n",
d352eeccec7e84 drivers/net/wireless/zydas/zd1211rw/zd_usb.c Gustavo A. R. Silva 2019-08-30 1637 rr->length, struct_size(regs, regs, count));
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1638 return false;
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1639 }
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1640
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1641 if (rr->length > sizeof(rr->buffer)) {
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1642 dev_dbg_f(zd_usb_dev(usb),
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1643 "error: actual length %d exceeds buffer size %zu\n",
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1644 rr->length, sizeof(rr->buffer));
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1645 return false;
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1646 }
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1647
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1648 for (i = 0; i < count; i++) {
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1649 struct reg_data *rd = ®s->regs[i];
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1650 if (rd->addr != req->addr[i]) {
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1651 dev_dbg_f(zd_usb_dev(usb),
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1652 "rd[%d] addr %#06hx expected %#06hx\n", i,
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1653 le16_to_cpu(rd->addr),
e85d0918b54fbd drivers/net/wireless/zd1211rw/zd_usb.c Daniel Drake 2006-06-02 1654 le16_to_cpu(req->addr[i]));
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1655 return false;
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1656 }
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1657 }
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1658
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1659 return true;
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1660 }
c900eff30a14ec drivers/net/wireless/zd1211rw/zd_usb.c Jussi Kivilinna 2011-06-20 1661
:::::: The code at line 1635 was first introduced by commit
:::::: e85d0918b54fbd9b38003752f7d665416b06edd8 [PATCH] ZyDAS ZD1211 USB-WLAN driver
:::::: TO: Daniel Drake <dsd@gentoo.org>
:::::: CC: Jeff Garzik <jeff@garzik.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 61473 bytes --]
^ permalink raw reply
* Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls
From: Markus Elfring @ 2019-08-31 9:15 UTC (permalink / raw)
To: Denis Efremov, Joe Perches
Cc: Andrew Morton, Anton Altaparmakov, Andy Whitcroft,
Boris Ostrovsky, Boris Pismenny, Darrick J. Wong, David S. Miller,
Dennis Dalessandro, Dmitry Torokhov, dri-devel,
Inaky Perez-Gonzalez, Jürgen Groß, Leon Romanovsky,
linux-arm-msm, linux-fsdevel, linux-input, linux-kernel,
linux-ntfs-dev, linux-rdma, linux-wimax, linux-xfs,
Mike Marciniszyn, netdev, Pali Rohár, Rob Clark,
Saeed Mahameed, Sean Paul, Alexander Viro, xen-devel,
Enrico Weigelt
In-Reply-To: <20190829165025.15750-1-efremov@linux.com>
> +# nested likely/unlikely calls
> + if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
> + WARN("LIKELY_MISUSE",
How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
in this regular expression?
Regards,
Markus
^ 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