* [PATCH net-next 0/4] net: dsa: add master interface
From: Vivien Didelot @ 2017-08-31 18:37 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot
Currently the SoC network interface (called master) to which a switch
fabric hangs, has its dsa_ptr pointing to a dsa_switch_tree instance.
This is not quite correct, because this interface is physically wired to
one of the switch ports (called CPU port), and because in a switch
fabric with multiple CPU ports, several master interfaces will point to
several CPU ports of the same dsa_switch_tree.
This patchset adds a new dsa_master structure to represent the pipe
between the SoC master interface and its switch CPU port. This structure
will store specific data such as the master ethtool_ops copy and the
tagging protocol used to pass frames with the associated slave ports.
The dsa_ptr is changed to a dsa_master instance, and each DSA slave now
has a pointer to a master port.
This is a step forward better control over the CPU conduit and support
for multiple CPU ports.
Vivien Didelot (4):
net: dsa: introduce dsa_master
net: dsa: move master ethtool ops in dsa_master
net: dsa: change dsa_ptr for a dsa_master
net: dsa: assign a master to slave ports
drivers/net/dsa/b53/b53_common.c | 4 +-
drivers/net/dsa/bcm_sf2.c | 8 +--
drivers/net/dsa/mt7530.c | 4 +-
drivers/net/dsa/mv88e6060.c | 2 +-
drivers/net/dsa/qca8k.c | 2 +-
include/linux/netdevice.h | 4 +-
include/net/dsa.h | 42 +++++------
net/dsa/Makefile | 2 +-
net/dsa/dsa.c | 34 +--------
net/dsa/dsa2.c | 38 +++++-----
net/dsa/dsa_priv.h | 24 +++----
net/dsa/legacy.c | 34 +++++----
net/dsa/master.c | 149 +++++++++++++++++++++++++++++++++++++++
net/dsa/slave.c | 117 +++++-------------------------
net/dsa/tag_brcm.c | 5 +-
net/dsa/tag_dsa.c | 3 +-
net/dsa/tag_edsa.c | 3 +-
net/dsa/tag_ksz.c | 5 +-
net/dsa/tag_lan9303.c | 6 +-
net/dsa/tag_mtk.c | 12 +---
net/dsa/tag_qca.c | 12 +---
net/dsa/tag_trailer.c | 5 +-
22 files changed, 265 insertions(+), 250 deletions(-)
create mode 100644 net/dsa/master.c
--
2.14.1
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-08-31 18:29 UTC (permalink / raw)
To: Mason, David Daney
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <ebee6e5d-5bc1-1c5b-b31d-6d50618d6074@free.fr>
On 08/31/2017 11:12 AM, Mason wrote:
> On 31/08/2017 19:53, Florian Fainelli wrote:
>> On 08/31/2017 10:49 AM, Mason wrote:
>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>> And the race is between phy_detach() setting phydev->attached_dev = NULL
>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>> netif_carrier_off().
>>>
>>> I must be missing something.
>>> (Since a thread cannot race against itself.)
>>>
>>> phy_disconnect calls phy_stop_machine which
>>> 1) stops the work queue from running in a separate thread
>>> 2) calls phy_state_machine *synchronously*
>>> which runs the PHY_HALTED case with everything well-defined
>>> end of phy_stop_machine
>>>
>>> phy_disconnect only then calls phy_detach()
>>> which makes future calls of phy_state_machine perilous.
>>>
>>> This all happens in the same thread, so I'm not yet
>>> seeing where the race happens?
>>
>> The race is as described in David's earlier email, so let's recap:
>>
>> Thread 1 Thread 2
>> phy_disconnect()
>> phy_stop_interrupts()
>> phy_stop_machine()
>> phy_state_machine()
>> -> queue_delayed_work()
>> phy_detach()
>> phy_state_machine()
>> -> netif_carrier_off()
>>
>> If phy_detach() finishes earlier than the workqueue had a chance to be
>> scheduled and process PHY_HALTED again, then we trigger the NULL pointer
>> de-reference.
>>
>> workqueues are not tasklets, the CPU scheduling them gets no guarantee
>> they will run on the same CPU.
>
> Something does not add up.
>
> The synchronous call to phy_state_machine() does:
>
> case PHY_HALTED:
> if (phydev->link) {
> phydev->link = 0;
> netif_carrier_off(phydev->attached_dev);
> phy_adjust_link(phydev);
> do_suspend = true;
> }
>
> then sets phydev->link = 0; therefore subsequent calls to
> phy_state_machin() will be no-op.
Actually you are right, once phydev->link is set to 0 these would become
no-ops. Still scratching my head as to what happens for David then...
>
> Also, queue_delayed_work() is only called in polling mode.
> David stated that he's using interrupt mode.
Right that's confusing too now. David can you check if you tree has:
49d52e8108a21749dc2114b924c907db43358984 ("net: phy: handle state
correctly in phy_stop_machine")
--
Florian
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-08-31 18:12 UTC (permalink / raw)
To: Florian Fainelli, David Daney
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <f74f1aad-3990-ae54-316f-751c3b15de41@gmail.com>
On 31/08/2017 19:53, Florian Fainelli wrote:
> On 08/31/2017 10:49 AM, Mason wrote:
>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>> And the race is between phy_detach() setting phydev->attached_dev = NULL
>>> and phy_state_machine() running in PHY_HALTED state and calling
>>> netif_carrier_off().
>>
>> I must be missing something.
>> (Since a thread cannot race against itself.)
>>
>> phy_disconnect calls phy_stop_machine which
>> 1) stops the work queue from running in a separate thread
>> 2) calls phy_state_machine *synchronously*
>> which runs the PHY_HALTED case with everything well-defined
>> end of phy_stop_machine
>>
>> phy_disconnect only then calls phy_detach()
>> which makes future calls of phy_state_machine perilous.
>>
>> This all happens in the same thread, so I'm not yet
>> seeing where the race happens?
>
> The race is as described in David's earlier email, so let's recap:
>
> Thread 1 Thread 2
> phy_disconnect()
> phy_stop_interrupts()
> phy_stop_machine()
> phy_state_machine()
> -> queue_delayed_work()
> phy_detach()
> phy_state_machine()
> -> netif_carrier_off()
>
> If phy_detach() finishes earlier than the workqueue had a chance to be
> scheduled and process PHY_HALTED again, then we trigger the NULL pointer
> de-reference.
>
> workqueues are not tasklets, the CPU scheduling them gets no guarantee
> they will run on the same CPU.
Something does not add up.
The synchronous call to phy_state_machine() does:
case PHY_HALTED:
if (phydev->link) {
phydev->link = 0;
netif_carrier_off(phydev->attached_dev);
phy_adjust_link(phydev);
do_suspend = true;
}
then sets phydev->link = 0; therefore subsequent calls to
phy_state_machin() will be no-op.
Also, queue_delayed_work() is only called in polling mode.
David stated that he's using interrupt mode.
Regards.
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-08-31 17:53 UTC (permalink / raw)
To: Mason, David Daney
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <d6a6b552-95a7-8353-54c8-fa804f9366a1@free.fr>
On 08/31/2017 10:49 AM, Mason wrote:
> On 31/08/2017 18:57, Florian Fainelli wrote:
>> And the race is between phy_detach() setting phydev->attached_dev = NULL
>> and phy_state_machine() running in PHY_HALTED state and calling
>> netif_carrier_off().
>
> I must be missing something.
> (Since a thread cannot race against itself.)
>
> phy_disconnect calls phy_stop_machine which
> 1) stops the work queue from running in a separate thread
> 2) calls phy_state_machine *synchronously*
> which runs the PHY_HALTED case with everything well-defined
> end of phy_stop_machine
>
> phy_disconnect only then calls phy_detach()
> which makes future calls of phy_state_machine perilous.
>
> This all happens in the same thread, so I'm not yet
> seeing where the race happens?
The race is as described in David's earlier email, so let's recap:
Thread 1 Thread 2
phy_disconnect()
phy_stop_interrupts()
phy_stop_machine()
phy_state_machine()
-> queue_delayed_work()
phy_detach()
phy_state_machine()
-> netif_carrier_off()
If phy_detach() finishes earlier than the workqueue had a chance to be
scheduled and process PHY_HALTED again, then we trigger the NULL pointer
de-reference.
workqueues are not tasklets, the CPU scheduling them gets no guarantee
they will run on the same CPU.
--
Florian
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-08-31 17:49 UTC (permalink / raw)
To: Florian Fainelli, David Daney
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <931bf454-81ff-94dc-82e6-bc2b889bd43a@gmail.com>
On 31/08/2017 18:57, Florian Fainelli wrote:
> And the race is between phy_detach() setting phydev->attached_dev = NULL
> and phy_state_machine() running in PHY_HALTED state and calling
> netif_carrier_off().
I must be missing something.
(Since a thread cannot race against itself.)
phy_disconnect calls phy_stop_machine which
1) stops the work queue from running in a separate thread
2) calls phy_state_machine *synchronously*
which runs the PHY_HALTED case with everything well-defined
end of phy_stop_machine
phy_disconnect only then calls phy_detach()
which makes future calls of phy_state_machine perilous.
This all happens in the same thread, so I'm not yet
seeing where the race happens?
Regards.
^ permalink raw reply
* Re: DSA mv88e6xxx RX frame errors and TCP/IP RX failure
From: David Miller @ 2017-08-31 17:44 UTC (permalink / raw)
To: andrew; +Cc: tharvey, netdev, vivien.didelot, linux-kernel, fugang.duan,
imirkin
In-Reply-To: <20170831174141.GP22289@lunn.ch>
From: Andrew Lunn <andrew@lunn.ch>
Date: Thu, 31 Aug 2017 19:41:41 +0200
> Please could you add
>
> fbbeefdd2104 ("net: fec: Allow reception of frames bigger than 1522 bytes")
>
> to stable.
Queued up, thanks.
^ permalink raw reply
* Re: DSA mv88e6xxx RX frame errors and TCP/IP RX failure
From: Andrew Lunn @ 2017-08-31 17:41 UTC (permalink / raw)
To: Tim Harvey, David Miller
Cc: netdev, Vivien Didelot, linux-kernel@vger.kernel.org, Fugang Duan,
Ilia Mirkin
In-Reply-To: <CAJ+vNU0dq-ULUj3PbQH_biGviJURj2z5McCXx_vYxcB0k94WkA@mail.gmail.com>
> > I did fix an issue recently with that. See
> >
> > commit fbbeefdd21049fcf9437c809da3828b210577f36
> > Author: Andrew Lunn <andrew@lunn.ch>
> > Date: Sun Jul 30 19:36:05 2017 +0200
> >
> > net: fec: Allow reception of frames bigger than 1522 bytes
> >
> > The FEC Receive Control Register has a 14 bit field indicating the
> > longest frame that may be received. It is being set to 1522. Frames
> > longer than this are discarded, but counted as being in error.
> >
> > When using DSA, frames from the switch has an additional header,
> > either 4 or 8 bytes if a Marvell switch is used. Thus a full MTU frame
> > of 1522 bytes received by the switch on a port becomes 1530 bytes when
> > passed to the host via the FEC interface.
> >
> > Change the maximum receive size to 2048 - 64, where 64 is the maximum
> > rx_alignment applied on the receive buffer for AVB capable FEC
> > cores. Use this value also for the maximum receive buffer size. The
> > driver is already allocating a receive SKB of 2048 bytes, so this
> > change should not have any significant effects.
> >
> > Tested on imx51, imx6, vf610.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> >
> >
> > However, this is was of an all/nothing problem. All frames with the
> > full MTU were getting dropped, where as i think you are only seeing a
> > few dropped?
> >
>
> Andrew,
>
> Indeed this does resolve the issue. I see a burst of FIFO overruns
> initially when receiving an iperf bandwidth test but that would be
> caused by the IMX6 errata and should be mitigated via pause frames.
> After that short burst I see no other errors and iperf works fine.
>
> Should we get this patch in the linux-stable tree for the 4.9 kernel?
Hi David
Please could you add
fbbeefdd2104 ("net: fec: Allow reception of frames bigger than 1522 bytes")
to stable.
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-08-31 17:35 UTC (permalink / raw)
To: David Daney, Florian Fainelli
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <e24693e8-d8ae-188a-2a38-c9a83fdc94e3@gmail.com>
On 31/08/2017 18:36, David Daney wrote:
> On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
>> On 31/08/2017 02:49, Florian Fainelli wrote:
>>
>>> This reverts commit 7ad813f208533cebfcc32d3d7474dc1677d1b09a ("net: phy:
>>> Correctly process PHY_HALTED in phy_stop_machine()") because it is
>>> creating the possibility for a NULL pointer dereference.
>>>
>>> David Daney provide the following call trace and diagram of events:
>>>
>>> When ndo_stop() is called we call:
>>>
>>> phy_disconnect()
>>> +---> phy_stop_interrupts() implies: phydev->irq = PHY_POLL;
>>
>> What does this mean?
>
> I meant that after the call to phy_stop_interrupts(), phydev->irq =
> PHY_POLL;
I must be missing something.
http://elixir.free-electrons.com/linux/latest/source/drivers/net/phy/phy.c#L868
phy_stop_interrupts() doesn't change phydev->irq right?
Only phy_start_interrupts() sets phydev->irq to
PHY_POLL if it cannot set up interrupt mode.
Regards.
^ permalink raw reply
* Re: [PATCH] wl1251: add a missing spin_lock_init()
From: David Miller @ 2017-08-31 17:33 UTC (permalink / raw)
To: pavel
Cc: kvalo, torvalds, xiyou.wangcong, akpm, netdev, linux-kernel,
linux-wireless
In-Reply-To: <20170831144743.GA21261@amd>
From: Pavel Machek <pavel@ucw.cz>
Date: Thu, 31 Aug 2017 16:47:43 +0200
> Dave, Linus -- can you still take the patch?
Pavel, please do not bypass maintainers like this.
It's really rude, and if you do things like that instead of
trying to work properly with us, your relationship with
these maintainers will suffer in the long term.
Thank you.
^ permalink raw reply
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection
From: Mike Galbraith @ 2017-08-31 17:19 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar,
Reshetova, Elena, Network Development
In-Reply-To: <CAGXu5jL8Ueuw-DLY+Hz8Jd7NgqgyaaN8OVDicFxmcjBhScebOA@mail.gmail.com>
On Thu, 2017-08-31 at 10:00 -0700, Kees Cook wrote:
>
> Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct:
>
> 4.8.5: WARN, eventual kernel hang
> 6.3.1, 7.0.1: WARN, but continues working
Yeah, that's correct. I find that troubling, simply because this gcc
version has been through one hell of a lot of kernels with me. Yeah, I
know, that doesn't exempt it from having bugs, but color me suspicious.
-Mike
^ permalink raw reply
* [PATCH][next] net/mlx4_core: fix incorrect size allocation for dev->caps.spec_qps
From: Colin King @ 2017-08-31 17:07 UTC (permalink / raw)
To: Tariq Toukan, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
Cc: kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
The current allocation for dev->caps.spec_qps is for the size of the
pointer and not the size of the actual mlx4_spec_qps structure. Fix
this by using the correct size. Also splint allocation over a few
lines to make it cppcheck clean on overly wide lines.
Detected by CoverityScan, CID#1455222 ("Wrong sizeof argument")
Fixes: c73c8b1e47ca ("net/mlx4_core: Dynamically allocate structs at mlx4_slave_cap")
Signed-off-by: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
drivers/net/ethernet/mellanox/mlx4/qp.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/qp.c b/drivers/net/ethernet/mellanox/mlx4/qp.c
index b16fc441609e..728a2fb1f5c0 100644
--- a/drivers/net/ethernet/mellanox/mlx4/qp.c
+++ b/drivers/net/ethernet/mellanox/mlx4/qp.c
@@ -845,8 +845,9 @@ int mlx4_init_qp_table(struct mlx4_dev *dev)
/* In mfunc, calculate proxy and tunnel qp offsets for the PF here,
* since the PF does not call mlx4_slave_caps */
- dev->caps.spec_qps = kcalloc(dev->caps.num_ports, sizeof(dev->caps.spec_qps), GFP_KERNEL);
-
+ dev->caps.spec_qps = kcalloc(dev->caps.num_ports,
+ sizeof(*dev->caps.spec_qps),
+ GFP_KERNEL);
if (!dev->caps.spec_qps) {
err = -ENOMEM;
goto err_mem;
--
2.14.1
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* virtio_net: ethtool supported link modes
From: Radu Rendec @ 2017-08-31 17:04 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang
Hello,
Looking at the code in virtnet_set_link_ksettings, it seems the speed
and duplex can be set to any valid value. The driver will "remember"
them and report them back in virtnet_get_link_ksettings.
However, the supported link modes (link_modes.supported in struct
ethtool_link_ksettings) is always 0, indicating that no speed/duplex
setting is supported.
Does it make more sense to set (at least a few of) the supported link
modes, such as 10baseT_Half ... 10000baseT_Full?
I would expect to see consistency between what is reported in
link_modes.supported and what can actually be set. Could you please
share your opinion on this?
Thank you,
Radu Rendec
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-08-31 17:03 UTC (permalink / raw)
To: Marc Gonzalez, David Daney
Cc: netdev, Geert Uytterhoeven, David Miller, Andrew Lunn,
Mans Rullgard, Mason
In-Reply-To: <f4bb5ac8-dae8-c0af-7aa6-e546fc0783fa@sigmadesigns.com>
On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
> On 31/08/2017 02:49, Florian Fainelli wrote:
>
>> This reverts commit 7ad813f208533cebfcc32d3d7474dc1677d1b09a ("net: phy:
>> Correctly process PHY_HALTED in phy_stop_machine()") because it is
>> creating the possibility for a NULL pointer dereference.
>>
>> David Daney provide the following call trace and diagram of events:
>>
>> When ndo_stop() is called we call:
>>
>> phy_disconnect()
>> +---> phy_stop_interrupts() implies: phydev->irq = PHY_POLL;
>
> What does this mean?
>
> On the contrary, phy_stop_interrupts() is only called when *not* polling.
>
> if (phydev->irq > 0)
> phy_stop_interrupts(phydev);
>
>> +---> phy_stop_machine()
>> | +---> phy_state_machine()
>> | +----> queue_delayed_work(): Work queued.
>
> You're referring to the fact that, at the end of phy_state_machine()
> (in polling mode) the code reschedules itself through:
>
> if (phydev->irq == PHY_POLL)
> queue_delayed_work(system_power_efficient_wq, &phydev->state_queue, PHY_STATE_TIME * HZ);
>
>> +--->phy_detach() implies: phydev->attached_dev = NULL;
>>
>> Now at a later time the queued work does:
>>
>> phy_state_machine()
>> +---->netif_carrier_off(phydev->attached_dev): Oh no! It is NULL:
>
> I tested a sequence of 500 link up / link down in polling mode,
> and saw no such issue. Race condition?
>
> For what case in phy_state_machine() is netif_carrier_off()
> being called? Surely not PHY_HALTED?
>
>
>> The original motivation for this change originated from Marc Gonzales
>> indicating that his network driver did not have its adjust_link callback
>> executing with phydev->link = 0 while he was expecting it.
>
> I expect the core to call phy_adjust_link() for link changes.
> This used to work back in 3.4 and was broken somewhere along
> the way.
If that was working correctly in 3.4 surely we can look at the diff and
figure out what changed, even maybe find the offending commit, can you
do that?
>
>> PHYLIB has never made any such guarantees ever because phy_stop() merely
>> just tells the workqueue to move into PHY_HALTED state which will happen
>> asynchronously.
>
> My original proposal was to fix the issue in the driver.
> I'll try locating it in my archives.
Yes I remember you telling that, by the way I don't think you ever
provided a clear explanation why this is absolutely necessary for your
driver though?
--
Florian
^ permalink raw reply
* Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection
From: Kees Cook @ 2017-08-31 17:00 UTC (permalink / raw)
To: Mike Galbraith
Cc: David S. Miller, Peter Zijlstra, LKML, Ingo Molnar,
Reshetova, Elena, Network Development
In-Reply-To: <1504187918.27500.16.camel@gmx.de>
On Thu, Aug 31, 2017 at 6:58 AM, Mike Galbraith <efault@gmx.de> wrote:
> On Wed, 2017-08-30 at 21:10 -0700, Kees Cook wrote:
>> On Wed, Aug 30, 2017 at 9:01 PM, Kees Cook <keescook@chromium.org> wrote:
>> > On Wed, Aug 30, 2017 at 8:12 PM, Mike Galbraith <efault@gmx.de> wrote:
>> >> On Wed, 2017-08-30 at 19:27 -0700, Kees Cook wrote:
>> >>
>> >>> Interesting! Can you try with 633547973ffc3 ("net: convert
>> >>> sk_buff.users from atomic_t to refcount_t") reverted? I'll see if
>> >>> running haveged will help me trigger this on my system...
>> >>
>> >> With that (plus 230cd1279d001 fix to it) reverted, vbox boots.
>> >
>> > Wonderful! Thank you so much for helping track this down.
>> >
>> > So, it seems that sk_buff.users will need some more special attention
>> > before we can convert it to refcount.
>> >
>> > x86-refcount will saturate with refcount_dec_and_test() if the result
>> > is negative. But that would mean at least starting at 0. FULL should
>> > have WARNed in this case, so I remain slightly confused why it was
>> > missed by FULL.
>>
>> Actually, if this is a race condition it's possible that FULL is slow
>> enough to miss it...
>>
>> I bet something briefly takes the refcount negative, and with
>> unchecked atomics, it come back up positive again during the race.
>> FULL may miss the race, and x86-refcount will catch it and saturate...
>
> (gdb) list *in6_dev_get+0x1e
> 0xffffffff8166d3de is in in6_dev_get (./arch/x86/include/asm/refcount.h:52).
> 47 : "cc", "cx");
> 48 }
> 49
> 50 static __always_inline void refcount_inc(refcount_t *r)
> 51 {
> 52 asm volatile(LOCK_PREFIX "incl %0\n\t"
> 53 REFCOUNT_CHECK_LT_ZERO
> 54 : [counter] "+m" (r->refs.counter)
> 55 : : "cc", "cx");
> 56
>
> gdb) list *in6_dev_get+0x10
> 0xffffffff8166d3d0 is in in6_dev_get (./include/net/addrconf.h:318).
> 313 {
> 314 struct inet6_dev *idev;
> 315
> 316 rcu_read_lock();
> 317 idev = rcu_dereference(dev->ip6_ptr);
> 318 if (idev)
> 319 refcount_inc(&idev->refcnt);
> 320 rcu_read_unlock();
> 321 return idev;
> 322
>
> That's from kernel with no revert, but your silent saturation patch
> still applied, AND built with gcc-6.3.1. Kernel traps, but it boots
> and works, as does kernel built with gcc-7.0.1. Remove your silent
> saturation patch, kernel doesn't notice a thing, just works.
>
> With gcc-4.8.5, trap means you're as good as dead, with the other two,
> trap means the intended. Compiler, constraints, dark elves.. pick one.
Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct:
4.8.5: WARN, eventual kernel hang
6.3.1, 7.0.1: WARN, but continues working
> Full first splat from bootable gcc-6.3.1 built kernel.
>
> [ 1.293962] NET: Registered protocol family 10
> [ 1.294635] refcount_t silent saturation at in6_dev_get+0x25/0x104 in swapper/0[1], uid/euid: 0/0
That's an _increment_ saturation? Which means the result must be
negative, so it started from least -2.
> [ 1.295616] ------------[ cut here ]------------
> [ 1.296120] WARNING: CPU: 0 PID: 1 at kernel/panic.c:612 refcount_error_report+0x94/0x9e
> [ 1.296950] Modules linked in:
> [ 1.297276] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.13.0.g152d54a-tip-default #53
> [ 1.299179] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
> [ 1.300743] task: ffff88013ab84040 task.stack: ffffc9000062c000
> [ 1.301825] RIP: 0010:refcount_error_report+0x94/0x9e
> [ 1.302804] RSP: 0018:ffffc9000062fc10 EFLAGS: 00010282
> [ 1.303791] RAX: 0000000000000055 RBX: ffffffff81a34274 RCX: ffffffff81c605e8
> [ 1.304991] RDX: 0000000000000001 RSI: 0000000000000096 RDI: 0000000000000246
> [ 1.306189] RBP: ffffc9000062fd58 R08: 0000000000000000 R09: 0000000000000175
> [ 1.307392] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88013ab84040
> [ 1.308583] R13: 0000000000000000 R14: 0000000000000004 R15: ffffffff81a256c8
> [ 1.309768] FS: 0000000000000000(0000) GS:ffff88013fc00000(0000) knlGS:0000000000000000
> [ 1.311052] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1.312100] CR2: 00007f4631fe8df0 CR3: 0000000137d09003 CR4: 00000000001606f0
> [ 1.313301] Call Trace:
> [ 1.314012] ex_handler_refcount+0x63/0x70
> [ 1.314893] fixup_exception+0x32/0x40
> [ 1.315737] do_trap+0x8c/0x170
> [ 1.316519] do_error_trap+0x70/0xd0
> [ 1.317340] ? in6_dev_get+0x23/0x104
> [ 1.318172] ? netlink_broadcast_filtered+0x2bd/0x430
> [ 1.319156] ? kmem_cache_alloc_trace+0xce/0x5d0
> [ 1.320098] ? set_debug_rodata+0x11/0x11
> [ 1.320964] invalid_op+0x1e/0x30
> [ 1.322520] RIP: 0010:in6_dev_get+0x25/0x104
> [ 1.323631] RSP: 0018:ffffc9000062fe00 EFLAGS: 00010202
> [ 1.324614] RAX: ffff880137de2400 RBX: ffff880137df4600 RCX: ffff880137de24f0
> [ 1.325793] RDX: ffff88013a5e4000 RSI: 00000000fffffe00 RDI: ffff88013a5e4000
> [ 1.326964] RBP: 00000000000000d1 R08: 0000000000000000 R09: ffff880137de7600
> [ 1.328150] R10: 0000000000000000 R11: ffff8801398a4df8 R12: 0000000000000000
> [ 1.329374] R13: ffffffff82137872 R14: 014200ca00000000 R15: 0000000000000000
> [ 1.330547] ? set_debug_rodata+0x11/0x11
> [ 1.331392] ip6_route_init_special_entries+0x2a/0x89
> [ 1.332369] addrconf_init+0x9e/0x203
> [ 1.333173] inet6_init+0x1af/0x365
> [ 1.333956] ? af_unix_init+0x4e/0x4e
> [ 1.334753] do_one_initcall+0x4e/0x190
> [ 1.335555] ? set_debug_rodata+0x11/0x11
> [ 1.336369] kernel_init_freeable+0x189/0x20e
> [ 1.337230] ? rest_init+0xd0/0xd0
> [ 1.337999] kernel_init+0xa/0xf7
> [ 1.338744] ret_from_fork+0x25/0x30
> [ 1.339500] Code: 48 8b 95 80 00 00 00 41 55 49 8d 8c 24 f0 0a 00 00 45 8b 84 24 10 09 00 00 41 89 c1 48 89 de 48 c7 c7 60 7a a3 81 e8 07 de 05 00 <0f> ff 58 5b 5d 41 5c 41 5d c3 0f 1f 44 00 00 55 48 89 e5 41 56
> [ 1.342243] ---[ end trace b5d40c0fccce776c ]---
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* [PATCH net-next v5 0/2] report TCP MD5 signing keys and addresses
From: Ivan Delalande @ 2017-08-31 16:59 UTC (permalink / raw)
To: David Miller; +Cc: Eric Dumazet, netdev, Ivan Delalande
Allow userspace to retrieve MD5 signature keys and addresses configured
on TCP sockets through inet_diag.
Thanks to Eric Dumazet and Stephen Hemminger for their useful
explanations and feedback.
v5: - memset the whole netlink payload after it has been nla_reserve-d
in tcp_diag_put_md5sig (a third memset had to be added for
tcpm_key so we might as well have just one for entire region).
- move the nla_total_size call from inet_sk_attr_size to the
idiag_get_aux_size defined by protocols as they could add multiple
netlink attributes,
- add check for net_admin in tcp_diag_get_aux_size.
v4: - add new struct tcp_diag_md5sig to report the data instead of
tcp_md5sig to avoid wasting 112 bytes on every tcpm_addr,
- memset tcpm_addr on IPv4 addresses to avoid leaks,
- style fix in inet_diag_dump_one_icsk.
v3: - rename inet_diag_*md5sig in tcp_diag.c to tcp_diag_* for
consistency,
- don't lock the socket in tcp_diag_put_md5sig,
- add checks on md5sig_count in tcp_diag_put_md5sig to not create
the netlink attribute if the list is empty, and to avoid overflows
or memory leaks if the list has changed in the meantime.
v2: - move changes to tcp_diag.c and extend inet_diag_handler to allow
protocols to provide additional data on INET_DIAG_INFO,
- lock socket before calling tcp_diag_put_md5sig.
I also have a patch for iproute2/ss to test this change, making it print
this new attribute. I'm planning to polish and send it if this series
gets applied.
Ivan Delalande (2):
inet_diag: allow protocols to provide additional data
tcp_diag: report TCP MD5 signing keys and addresses
include/linux/inet_diag.h | 7 +++
include/uapi/linux/inet_diag.h | 1 +
include/uapi/linux/tcp.h | 9 ++++
net/ipv4/inet_diag.c | 22 +++++++--
net/ipv4/tcp_diag.c | 109 ++++++++++++++++++++++++++++++++++++++---
5 files changed, 138 insertions(+), 10 deletions(-)
--
2.14.1
^ permalink raw reply
* [PATCH net-next v5 2/2] tcp_diag: report TCP MD5 signing keys and addresses
From: Ivan Delalande @ 2017-08-31 16:59 UTC (permalink / raw)
To: David Miller; +Cc: Eric Dumazet, netdev, Ivan Delalande
In-Reply-To: <20170831165939.5121-1-colona@arista.com>
Report TCP MD5 (RFC2385) signing keys, addresses and address prefixes to
processes with CAP_NET_ADMIN requesting INET_DIAG_INFO. Currently it is
not possible to retrieve these from the kernel once they have been
configured on sockets.
Signed-off-by: Ivan Delalande <colona@arista.com>
---
include/uapi/linux/inet_diag.h | 1 +
include/uapi/linux/tcp.h | 9 ++++
net/ipv4/tcp_diag.c | 109 ++++++++++++++++++++++++++++++++++++++---
3 files changed, 113 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
index 678496897a68..f52ff62bfabe 100644
--- a/include/uapi/linux/inet_diag.h
+++ b/include/uapi/linux/inet_diag.h
@@ -143,6 +143,7 @@ enum {
INET_DIAG_MARK,
INET_DIAG_BBRINFO,
INET_DIAG_CLASS_ID,
+ INET_DIAG_MD5SIG,
__INET_DIAG_MAX,
};
diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
index 030e594bab45..15c25eccab2b 100644
--- a/include/uapi/linux/tcp.h
+++ b/include/uapi/linux/tcp.h
@@ -256,4 +256,13 @@ struct tcp_md5sig {
__u8 tcpm_key[TCP_MD5SIG_MAXKEYLEN]; /* key (binary) */
};
+/* INET_DIAG_MD5SIG */
+struct tcp_diag_md5sig {
+ __u8 tcpm_family;
+ __u8 tcpm_prefixlen;
+ __u16 tcpm_keylen;
+ __be32 tcpm_addr[4];
+ __u8 tcpm_key[TCP_MD5SIG_MAXKEYLEN];
+};
+
#endif /* _UAPI_LINUX_TCP_H */
diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
index a748c74aa8b7..abbf0edcf6c2 100644
--- a/net/ipv4/tcp_diag.c
+++ b/net/ipv4/tcp_diag.c
@@ -16,6 +16,7 @@
#include <linux/tcp.h>
+#include <net/netlink.h>
#include <net/tcp.h>
static void tcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
@@ -36,6 +37,100 @@ static void tcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
tcp_get_info(sk, info);
}
+#ifdef CONFIG_TCP_MD5SIG
+static void tcp_diag_md5sig_fill(struct tcp_diag_md5sig *info,
+ const struct tcp_md5sig_key *key)
+{
+ info->tcpm_family = key->family;
+ info->tcpm_prefixlen = key->prefixlen;
+ info->tcpm_keylen = key->keylen;
+ memcpy(info->tcpm_key, key->key, key->keylen);
+
+ if (key->family == AF_INET)
+ info->tcpm_addr[0] = key->addr.a4.s_addr;
+ #if IS_ENABLED(CONFIG_IPV6)
+ else if (key->family == AF_INET6)
+ memcpy(&info->tcpm_addr, &key->addr.a6,
+ sizeof(info->tcpm_addr));
+ #endif
+}
+
+static int tcp_diag_put_md5sig(struct sk_buff *skb,
+ const struct tcp_md5sig_info *md5sig)
+{
+ const struct tcp_md5sig_key *key;
+ struct tcp_diag_md5sig *info;
+ struct nlattr *attr;
+ int md5sig_count = 0;
+
+ hlist_for_each_entry_rcu(key, &md5sig->head, node)
+ md5sig_count++;
+ if (md5sig_count == 0)
+ return 0;
+
+ attr = nla_reserve(skb, INET_DIAG_MD5SIG,
+ md5sig_count * sizeof(struct tcp_diag_md5sig));
+ if (!attr)
+ return -EMSGSIZE;
+
+ info = nla_data(attr);
+ memset(info, 0, md5sig_count * sizeof(struct tcp_diag_md5sig));
+ hlist_for_each_entry_rcu(key, &md5sig->head, node) {
+ tcp_diag_md5sig_fill(info++, key);
+ if (--md5sig_count == 0)
+ break;
+ }
+
+ return 0;
+}
+#endif
+
+static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
+ struct sk_buff *skb)
+{
+#ifdef CONFIG_TCP_MD5SIG
+ if (net_admin) {
+ struct tcp_md5sig_info *md5sig;
+ int err = 0;
+
+ rcu_read_lock();
+ md5sig = rcu_dereference(tcp_sk(sk)->md5sig_info);
+ if (md5sig)
+ err = tcp_diag_put_md5sig(skb, md5sig);
+ rcu_read_unlock();
+ if (err < 0)
+ return err;
+ }
+#endif
+
+ return 0;
+}
+
+static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
+{
+ size_t size = 0;
+
+#ifdef CONFIG_TCP_MD5SIG
+ if (net_admin && sk_fullsock(sk)) {
+ const struct tcp_md5sig_info *md5sig;
+ const struct tcp_md5sig_key *key;
+ size_t md5sig_count = 0;
+
+ rcu_read_lock();
+ md5sig = rcu_dereference(tcp_sk(sk)->md5sig_info);
+ if (md5sig) {
+ hlist_for_each_entry_rcu(key, &md5sig->head, node)
+ md5sig_count++;
+ }
+ rcu_read_unlock();
+ size += nla_total_size(md5sig_count *
+ sizeof(struct tcp_diag_md5sig));
+ }
+#endif
+
+ return size;
+}
+
static void tcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
const struct inet_diag_req_v2 *r, struct nlattr *bc)
{
@@ -68,13 +163,15 @@ static int tcp_diag_destroy(struct sk_buff *in_skb,
#endif
static const struct inet_diag_handler tcp_diag_handler = {
- .dump = tcp_diag_dump,
- .dump_one = tcp_diag_dump_one,
- .idiag_get_info = tcp_diag_get_info,
- .idiag_type = IPPROTO_TCP,
- .idiag_info_size = sizeof(struct tcp_info),
+ .dump = tcp_diag_dump,
+ .dump_one = tcp_diag_dump_one,
+ .idiag_get_info = tcp_diag_get_info,
+ .idiag_get_aux = tcp_diag_get_aux,
+ .idiag_get_aux_size = tcp_diag_get_aux_size,
+ .idiag_type = IPPROTO_TCP,
+ .idiag_info_size = sizeof(struct tcp_info),
#ifdef CONFIG_INET_DIAG_DESTROY
- .destroy = tcp_diag_destroy,
+ .destroy = tcp_diag_destroy,
#endif
};
--
2.14.1
^ permalink raw reply related
* [PATCH net-next v5 1/2] inet_diag: allow protocols to provide additional data
From: Ivan Delalande @ 2017-08-31 16:59 UTC (permalink / raw)
To: David Miller; +Cc: Eric Dumazet, netdev, Ivan Delalande
In-Reply-To: <20170831165939.5121-1-colona@arista.com>
Extend inet_diag_handler to allow individual protocols to report
additional data on INET_DIAG_INFO through idiag_get_aux. The size
can be dynamic and is computed by idiag_get_aux_size.
Signed-off-by: Ivan Delalande <colona@arista.com>
---
include/linux/inet_diag.h | 7 +++++++
net/ipv4/inet_diag.c | 22 ++++++++++++++++++----
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/include/linux/inet_diag.h b/include/linux/inet_diag.h
index 65da430e260f..ee251c585854 100644
--- a/include/linux/inet_diag.h
+++ b/include/linux/inet_diag.h
@@ -25,6 +25,13 @@ struct inet_diag_handler {
struct inet_diag_msg *r,
void *info);
+ int (*idiag_get_aux)(struct sock *sk,
+ bool net_admin,
+ struct sk_buff *skb);
+
+ size_t (*idiag_get_aux_size)(struct sock *sk,
+ bool net_admin);
+
int (*destroy)(struct sk_buff *in_skb,
const struct inet_diag_req_v2 *req);
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 67325d5832d7..c9c35b61a027 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -93,8 +93,17 @@ void inet_diag_msg_common_fill(struct inet_diag_msg *r, struct sock *sk)
}
EXPORT_SYMBOL_GPL(inet_diag_msg_common_fill);
-static size_t inet_sk_attr_size(void)
+static size_t inet_sk_attr_size(struct sock *sk,
+ const struct inet_diag_req_v2 *req,
+ bool net_admin)
{
+ const struct inet_diag_handler *handler;
+ size_t aux = 0;
+
+ handler = inet_diag_table[req->sdiag_protocol];
+ if (handler && handler->idiag_get_aux_size)
+ aux = handler->idiag_get_aux_size(sk, net_admin);
+
return nla_total_size(sizeof(struct tcp_info))
+ nla_total_size(1) /* INET_DIAG_SHUTDOWN */
+ nla_total_size(1) /* INET_DIAG_TOS */
@@ -105,6 +114,7 @@ static size_t inet_sk_attr_size(void)
+ nla_total_size(SK_MEMINFO_VARS * sizeof(u32))
+ nla_total_size(TCP_CA_NAME_MAX)
+ nla_total_size(sizeof(struct tcpvegas_info))
+ + aux
+ 64;
}
@@ -260,6 +270,10 @@ int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk,
handler->idiag_get_info(sk, r, info);
+ if (ext & (1 << (INET_DIAG_INFO - 1)) && handler->idiag_get_aux)
+ if (handler->idiag_get_aux(sk, net_admin, skb) < 0)
+ goto errout;
+
if (sk->sk_state < TCP_TIME_WAIT) {
union tcp_cc_info info;
size_t sz = 0;
@@ -449,6 +463,7 @@ int inet_diag_dump_one_icsk(struct inet_hashinfo *hashinfo,
const struct nlmsghdr *nlh,
const struct inet_diag_req_v2 *req)
{
+ bool net_admin = netlink_net_capable(in_skb, CAP_NET_ADMIN);
struct net *net = sock_net(in_skb->sk);
struct sk_buff *rep;
struct sock *sk;
@@ -458,7 +473,7 @@ int inet_diag_dump_one_icsk(struct inet_hashinfo *hashinfo,
if (IS_ERR(sk))
return PTR_ERR(sk);
- rep = nlmsg_new(inet_sk_attr_size(), GFP_KERNEL);
+ rep = nlmsg_new(inet_sk_attr_size(sk, req, net_admin), GFP_KERNEL);
if (!rep) {
err = -ENOMEM;
goto out;
@@ -467,8 +482,7 @@ int inet_diag_dump_one_icsk(struct inet_hashinfo *hashinfo,
err = sk_diag_fill(sk, rep, req,
sk_user_ns(NETLINK_CB(in_skb).sk),
NETLINK_CB(in_skb).portid,
- nlh->nlmsg_seq, 0, nlh,
- netlink_net_capable(in_skb, CAP_NET_ADMIN));
+ nlh->nlmsg_seq, 0, nlh, net_admin);
if (err < 0) {
WARN_ON(err == -EMSGSIZE);
nlmsg_free(rep);
--
2.14.1
^ permalink raw reply related
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-08-31 16:57 UTC (permalink / raw)
To: David Daney, Marc Gonzalez
Cc: netdev, Geert Uytterhoeven, David Miller, Andrew Lunn,
Mans Rullgard, Mason
In-Reply-To: <e24693e8-d8ae-188a-2a38-c9a83fdc94e3@gmail.com>
On 08/31/2017 09:36 AM, David Daney wrote:
> On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
>> On 31/08/2017 02:49, Florian Fainelli wrote:
>>
>>> This reverts commit 7ad813f208533cebfcc32d3d7474dc1677d1b09a ("net: phy:
>>> Correctly process PHY_HALTED in phy_stop_machine()") because it is
>>> creating the possibility for a NULL pointer dereference.
>>>
>>> David Daney provide the following call trace and diagram of events:
>>>
>>> When ndo_stop() is called we call:
>>>
>>> phy_disconnect()
>>> +---> phy_stop_interrupts() implies: phydev->irq = PHY_POLL;
>>
>> What does this mean?
>
> I meant that after the call to phy_stop_interrupts(), phydev->irq =
> PHY_POLL;
>
>
>>
>> On the contrary, phy_stop_interrupts() is only called when *not* polling.
>
> That is the case I have. We are using interrupts from the phy.
>
>
>>
>> if (phydev->irq > 0)
>> phy_stop_interrupts(phydev);
>>
>>> +---> phy_stop_machine()
>>> | +---> phy_state_machine()
>>> | +----> queue_delayed_work(): Work queued.
>>
>> You're referring to the fact that, at the end of phy_state_machine()
>> (in polling mode) the code reschedules itself through:
>>
>> if (phydev->irq == PHY_POLL)
>> queue_delayed_work(system_power_efficient_wq,
>> &phydev->state_queue, PHY_STATE_TIME * HZ);
>
> Exactly. The call to phy_disconnect() ensures that there are no more
> interrupts and also that phydev->irq = PHY_POLL
>
> The call to cancel_delayed_work_sync() at the top of phy_stop_machine()
> was meant to ensure that phy_state_machine() was never run again. No
> interrupts + no queued work means that it should be save to do...
>
>>
>>> +--->phy_detach() implies: phydev->attached_dev = NULL;
>
> The problem is that by calling phy_state_machine() again (which the
> offending patch added) we now have work scheduled that will try to
> dereference the pointer that was set to NULL as a result of the
> phy_detach()
And the race is between phy_detach() setting phydev->attached_dev = NULL
and phy_state_machine() running in PHY_HALTED state and calling
netif_carrier_off().
>
>
>>>
>>> Now at a later time the queued work does:
>>>
>>> phy_state_machine()
>>> +---->netif_carrier_off(phydev->attached_dev): Oh no! It is NULL:
>>
>> I tested a sequence of 500 link up / link down in polling mode,
>> and saw no such issue. Race condition?
>>
>
> You were lucky.
I too tested this a number of times on a 2 core and 4 core system, but
the race is there, both of us just were lucky enough we did not see any
crash. I suspect the race is easier to reproduce on a (at least 12 core)
system with possibly a higher clock speed.
>
>> For what case in phy_state_machine() is netif_carrier_off()
>> being called? Surely not PHY_HALTED?
>>
>
> The phy can be in a variety of states. It is connected to something
> outside of the system that we don't control, so you cannot assume any
> particular state. We must have code that doesn't crash the system no
> matter what state the phy is in.
>
> I suspect, but have not checked, that the phy is in PHY_RUNNING. I
> think that means that because this patch turned the state machine back
> on, it will start transitioning through PHY_UP, PHY_AN, ... and
> eventually get to the crash we see because phydev->attached_dev = NULL
I actually think the PHY remains in PHY_HALTED but just re-schedules
itself and keeps being in PHY_HALTED again until a call to phy_resume or
phy_start() moves it back to another state. This is largely inefficient,
and we should look into using the patch I posted yesterday which would
prevent a re-schedule when moved to PHY_HALTED:
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index d0626bf5c540..78168e19bd5d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1234,7 +1234,7 @@ void phy_state_machine(struct work_struct *work)
* PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
* between states from phy_mac_interrupt()
*/
- if (phydev->irq == PHY_POLL)
+ if (phydev->irq == PHY_POLL && phydev->state != PHY_HALTED)
queue_delayed_work(system_power_efficient_wq,
&phydev->state_queue,
PHY_STATE_TIME * HZ);
}
--
Florian
^ permalink raw reply related
* Re: [PATCH net] ipv4: Don't override return code from ip_route_input_noref()
From: Wei Wang @ 2017-08-31 16:39 UTC (permalink / raw)
To: Stefano Brivio
Cc: David S . Miller, Linux Kernel Network Developers,
Sabrina Dubroca
In-Reply-To: <c4d8970306e614ac139cf2f33ab5e6bb73b30154.1504194307.git.sbrivio@redhat.com>
> After ip_route_input() calls ip_route_input_noref(), another
> check on skb_dst() is done, but if this fails, we shouldn't
> override the return code from ip_route_input_noref(), as it
> could have been more specific (i.e. -EHOSTUNREACH).
>
> This also saves one call to skb_dst_force_safe() and one to
> skb_dst() in case the ip_route_input_noref() check fails.
>
> Reported-by: Sabrina Dubroca <sdubroca@redhat.com>
> Fixes: ad65a2f05695 ("ipv4: call dst_hold_safe() properly")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Acked-by: Wei Wang <weiwan@google.com>
On Thu, Aug 31, 2017 at 9:11 AM, Stefano Brivio <sbrivio@redhat.com> wrote:
> After ip_route_input() calls ip_route_input_noref(), another
> check on skb_dst() is done, but if this fails, we shouldn't
> override the return code from ip_route_input_noref(), as it
> could have been more specific (i.e. -EHOSTUNREACH).
>
> This also saves one call to skb_dst_force_safe() and one to
> skb_dst() in case the ip_route_input_noref() check fails.
>
> Reported-by: Sabrina Dubroca <sdubroca@redhat.com>
> Fixes: ad65a2f05695 ("ipv4: call dst_hold_safe() properly")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> include/net/route.h | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/route.h b/include/net/route.h
> index cb0a76d9dde1..1b09a9368c68 100644
> --- a/include/net/route.h
> +++ b/include/net/route.h
> @@ -189,10 +189,11 @@ static inline int ip_route_input(struct sk_buff *skb, __be32 dst, __be32 src,
>
> rcu_read_lock();
> err = ip_route_input_noref(skb, dst, src, tos, devin);
> - if (!err)
> + if (!err) {
> skb_dst_force_safe(skb);
> - if (!skb_dst(skb))
> - err = -EINVAL;
> + if (!skb_dst(skb))
> + err = -EINVAL;
> + }
> rcu_read_unlock();
>
> return err;
> --
> 2.9.4
>
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: David Daney @ 2017-08-31 16:36 UTC (permalink / raw)
To: Marc Gonzalez, Florian Fainelli
Cc: netdev, Geert Uytterhoeven, David Miller, Andrew Lunn,
Mans Rullgard, Mason
In-Reply-To: <f4bb5ac8-dae8-c0af-7aa6-e546fc0783fa@sigmadesigns.com>
On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
> On 31/08/2017 02:49, Florian Fainelli wrote:
>
>> This reverts commit 7ad813f208533cebfcc32d3d7474dc1677d1b09a ("net: phy:
>> Correctly process PHY_HALTED in phy_stop_machine()") because it is
>> creating the possibility for a NULL pointer dereference.
>>
>> David Daney provide the following call trace and diagram of events:
>>
>> When ndo_stop() is called we call:
>>
>> phy_disconnect()
>> +---> phy_stop_interrupts() implies: phydev->irq = PHY_POLL;
>
> What does this mean?
I meant that after the call to phy_stop_interrupts(), phydev->irq =
PHY_POLL;
>
> On the contrary, phy_stop_interrupts() is only called when *not* polling.
That is the case I have. We are using interrupts from the phy.
>
> if (phydev->irq > 0)
> phy_stop_interrupts(phydev);
>
>> +---> phy_stop_machine()
>> | +---> phy_state_machine()
>> | +----> queue_delayed_work(): Work queued.
>
> You're referring to the fact that, at the end of phy_state_machine()
> (in polling mode) the code reschedules itself through:
>
> if (phydev->irq == PHY_POLL)
> queue_delayed_work(system_power_efficient_wq, &phydev->state_queue, PHY_STATE_TIME * HZ);
Exactly. The call to phy_disconnect() ensures that there are no more
interrupts and also that phydev->irq = PHY_POLL
The call to cancel_delayed_work_sync() at the top of phy_stop_machine()
was meant to ensure that phy_state_machine() was never run again. No
interrupts + no queued work means that it should be save to do...
>
>> +--->phy_detach() implies: phydev->attached_dev = NULL;
The problem is that by calling phy_state_machine() again (which the
offending patch added) we now have work scheduled that will try to
dereference the pointer that was set to NULL as a result of the phy_detach()
>>
>> Now at a later time the queued work does:
>>
>> phy_state_machine()
>> +---->netif_carrier_off(phydev->attached_dev): Oh no! It is NULL:
>
> I tested a sequence of 500 link up / link down in polling mode,
> and saw no such issue. Race condition?
>
You were lucky.
> For what case in phy_state_machine() is netif_carrier_off()
> being called? Surely not PHY_HALTED?
>
The phy can be in a variety of states. It is connected to something
outside of the system that we don't control, so you cannot assume any
particular state. We must have code that doesn't crash the system no
matter what state the phy is in.
I suspect, but have not checked, that the phy is in PHY_RUNNING. I
think that means that because this patch turned the state machine back
on, it will start transitioning through PHY_UP, PHY_AN, ... and
eventually get to the crash we see because phydev->attached_dev = NULL
>
>> The original motivation for this change originated from Marc Gonzales
>> indicating that his network driver did not have its adjust_link callback
>> executing with phydev->link = 0 while he was expecting it.
>
> I expect the core to call phy_adjust_link() for link changes.
> This used to work back in 3.4 and was broken somewhere along
> the way.
>
>> PHYLIB has never made any such guarantees ever because phy_stop() merely
>> just tells the workqueue to move into PHY_HALTED state which will happen
>> asynchronously.
>
> My original proposal was to fix the issue in the driver.
> I'll try locating it in my archives.
>
> Regards.
>
^ permalink raw reply
* [PATCH][next] net/mlx4_core: fix memory leaks on error exit path
From: Colin King @ 2017-08-31 16:30 UTC (permalink / raw)
To: Tariq Toukan, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
Cc: kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
The structures hca_param and func_cap are not being kfree'd on an error
exit path causing two memory leaks. Fix this by jumping to the existing
free memory error exit path.
Detected by CoverityScan, CID#1455219, CID#1455224 ("Resource Leak")
Fixes: c73c8b1e47ca ("net/mlx4_core: Dynamically allocate structs at mlx4_slave_cap")
Signed-off-by: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
drivers/net/ethernet/mellanox/mlx4/main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index 1c92101b3ec2..d46f3283ec36 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -977,7 +977,8 @@ static int mlx4_slave_cap(struct mlx4_dev *dev)
if (dev->caps.num_ports > MLX4_MAX_PORTS) {
mlx4_err(dev, "HCA has %d ports, but we only support %d, aborting\n",
dev->caps.num_ports, MLX4_MAX_PORTS);
- return -ENODEV;
+ err = -ENODEV;
+ goto free_mem;
}
mlx4_replace_zero_macs(dev);
--
2.14.1
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [RFC PATCH] net: frag limit checks need to use percpu_counter_compare
From: Michal Kubecek @ 2017-08-31 16:23 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: liujian56, netdev, Florian Westphal
In-Reply-To: <150417481955.28907.15567119824187929000.stgit@firesoul>
On Thu, Aug 31, 2017 at 12:20:19PM +0200, Jesper Dangaard Brouer wrote:
> To: Liujian can you please test this patch?
> I want to understand if using __percpu_counter_compare() solves
> the problem correctness wise (even-though this will be slower
> than using a simple atomic_t on your big system).
>
> Fix bug in fragmentation codes use of the percpu_counter API, that
> cause issues on systems with many CPUs.
>
> The frag_mem_limit() just reads the global counter (fbc->count),
> without considering other CPUs can have upto batch size (130K) that
> haven't been subtracted yet. Due to the 3MBytes lower thresh limit,
> this become dangerous at >=24 CPUs (3*1024*1024/130000=24).
>
> The __percpu_counter_compare() does the right thing, and takes into
> account the number of (online) CPUs and batch size, to account for
> this and call __percpu_counter_sum() when needed.
>
> On systems with many CPUs this will unfortunately always result in the
> heavier fully locked __percpu_counter_sum() which touch the
> per_cpu_ptr of all (online) CPUs.
>
> On systems with a smaller number of CPUs this solution is also not
> optimal, because __percpu_counter_compare()/__percpu_counter_sum()
> doesn't help synchronize the global counter.
> Florian Westphal have an idea of adding some counter sync points,
> which should help address this issue.
> ---
> include/net/inet_frag.h | 16 ++++++++++++++--
> net/ipv4/inet_fragment.c | 6 +++---
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
> index 6fdcd2427776..b586e320783d 100644
> --- a/include/net/inet_frag.h
> +++ b/include/net/inet_frag.h
> @@ -147,9 +147,21 @@ static inline bool inet_frag_evicting(struct inet_frag_queue *q)
> */
> static unsigned int frag_percpu_counter_batch = 130000;
>
> -static inline int frag_mem_limit(struct netns_frags *nf)
> +static inline bool frag_mem_over_limit(struct netns_frags *nf, int thresh)
> {
> - return percpu_counter_read(&nf->mem);
> + /* When reading counter here, __percpu_counter_compare() call
> + * will invoke __percpu_counter_sum() when needed. Which
> + * depend on num_online_cpus()*batch size, as each CPU can
> + * potentential can hold a batch count.
> + *
> + * With many CPUs this heavier sum operation will
> + * unfortunately always occur.
> + */
> + if (__percpu_counter_compare(&nf->mem, thresh,
> + frag_percpu_counter_batch) > 0)
> + return true;
> + else
> + return false;
> }
>
> static inline void sub_frag_mem_limit(struct netns_frags *nf, int i)
> diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
> index 96e95e83cc61..ee2cf56900e6 100644
> --- a/net/ipv4/inet_fragment.c
> +++ b/net/ipv4/inet_fragment.c
> @@ -120,7 +120,7 @@ static void inet_frag_secret_rebuild(struct inet_frags *f)
> static bool inet_fragq_should_evict(const struct inet_frag_queue *q)
> {
> return q->net->low_thresh == 0 ||
> - frag_mem_limit(q->net) >= q->net->low_thresh;
> + frag_mem_over_limit(q->net, q->net->low_thresh);
> }
>
> static unsigned int
> @@ -355,7 +355,7 @@ static struct inet_frag_queue *inet_frag_alloc(struct netns_frags *nf,
> {
> struct inet_frag_queue *q;
>
> - if (!nf->high_thresh || frag_mem_limit(nf) > nf->high_thresh) {
> + if (!nf->high_thresh || frag_mem_over_limit(nf, nf->high_thresh)) {
> inet_frag_schedule_worker(f);
> return NULL;
> }
If we go this way (which would IMHO require some benchmarks to make sure
it doesn't harm performance too much) we can drop the explicit checks
for zero thresholds which were added to work around the unreliability of
fast checks of percpu counters (or at least the second one was by commit
30759219f562 ("net: disable fragment reassembly if high_thresh is zero").
Michal Kubecek
> @@ -396,7 +396,7 @@ struct inet_frag_queue *inet_frag_find(struct netns_frags *nf,
> struct inet_frag_queue *q;
> int depth = 0;
>
> - if (frag_mem_limit(nf) > nf->low_thresh)
> + if (frag_mem_over_limit(nf, nf->low_thresh))
> inet_frag_schedule_worker(f);
>
> hash &= (INETFRAGS_HASHSZ - 1);
>
^ permalink raw reply
* Re: [PATCH net-next] samples/bpf: Fix compilation issue in redirect dummy program
From: Daniel Borkmann @ 2017-08-31 16:20 UTC (permalink / raw)
To: Y Song
Cc: Jesper Dangaard Brouer, Tariq Toukan, David S. Miller, netdev,
Eran Ben Elisha, Alexei Starovoitov
In-Reply-To: <CAH3MdRVszTVtjruq=-rh26jBtY9evuv7yBz37Ydb8EWSexL+zQ@mail.gmail.com>
On 08/31/2017 05:54 PM, Y Song wrote:
> On Thu, Aug 31, 2017 at 4:43 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 08/31/2017 01:27 PM, Jesper Dangaard Brouer wrote:
>>> On Thu, 31 Aug 2017 14:16:39 +0300
>>> Tariq Toukan <tariqt@mellanox.com> wrote:
>>>
>>>> Fix compilation error below:
>>>>
>>>> $ make samples/bpf/
>>>>
>>>> LLVM ERROR: 'xdp_redirect_dummy' label emitted multiple times to
>>>> assembly file
>>>> make[1]: *** [samples/bpf/xdp_redirect_kern.o] Error 1
>>>> make: *** [samples/bpf/] Error 2
>>>>
>>>> Fixes: 306da4e685b4 ("samples/bpf: xdp_redirect load XDP dummy prog on TX
>>>> device")
>>>> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
>>>> ---
>>>
>>> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>
>> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
>>
>>> What LLVM/clang version do you use?
>>>
>>> I don't see this compile error, and I have:
>>> $ clang --version
>>> clang version 3.9.1 (tags/RELEA
>>
>> I'm seeing the error as well with a fairly recent LLVM from git
>> tree (6.0.0git-2d810c2).
>>
>> Looks like the llvm error is triggered when section name and
>> the function name for XDP prog is the same. Changing either the
>> function or the section name right above resolves the issue. If
>> such error didn't trigger on older versions, people could be
>> using such naming scheme as done here, so seems to me like a
>> regression on LLVM side we might need to look at ...
>
> Martin fixed a similar bug earlier:
> =====
> commit a2e8bbd2ef5457485f00b6b947bbbfa2778e5b1e
> Author: Martin KaFai Lau <kafai@fb.com>
> Date: Thu Jun 8 22:30:17 2017 -0700
>
> bpf: Fix test_obj_id.c for llvm 5.0
>
> llvm 5.0 does not like the section name and the function name
> to be the same:
> ...
> =====
Yeah indeed.
> gcc also has this behavior. Section name is treated as global
> and hence cannot collide with a function name...
Okay, so seems at least 3.9.1 treated this slightly different then
where it didn't cause a collision.
^ permalink raw reply
* Re: [PATCH net-next] bridge: add tracepoint in br_fdb_update
From: Jesper Dangaard Brouer @ 2017-08-31 16:20 UTC (permalink / raw)
To: David Ahern
Cc: Roopa Prabhu, davem@davemloft.net, netdev@vger.kernel.org,
Nikolay Aleksandrov, Florian Fainelli, Andrew Lunn, bridge,
brouer, Arnaldo Carvalho de Melo, Peter Zijlstra
In-Reply-To: <a9349049-bfd7-b6c0-d1c7-2f70b0b0ab11@gmail.com>
On Thu, 31 Aug 2017 09:30:05 -0600
David Ahern <dsahern@gmail.com> wrote:
> On 8/31/17 9:21 AM, Roopa Prabhu wrote:
> > On Thu, Aug 31, 2017 at 5:38 AM, Jesper Dangaard Brouer
> > <brouer@redhat.com> wrote:
> >> On Wed, 30 Aug 2017 22:18:13 -0700
> >> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
> >>
> >>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> >>>
> >>> This extends bridge fdb table tracepoints to also cover
> >>> learned fdb entries in the br_fdb_update path. Note that
> >>> unlike other tracepoints I have moved this to when the fdb
> >>> is modified because this is in the datapath and can generate
> >>> a lot of noise in the trace output. br_fdb_update is also called
> >>> from added_by_user context in the NTF_USE case which is already
> >>> traced ..hence the !added_by_user check.
> >>>
> >>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> >>> ---
> >>> include/trace/events/bridge.h | 31 +++++++++++++++++++++++++++++++
> >>> net/bridge/br_fdb.c | 5 ++++-
> >>> net/core/net-traces.c | 1 +
> >>> 3 files changed, 36 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/trace/events/bridge.h b/include/trace/events/bridge.h
> >>> index 0f1cde0..1bee3e7 100644
> >>> --- a/include/trace/events/bridge.h
> >>> +++ b/include/trace/events/bridge.h
> >>> @@ -92,6 +92,37 @@ TRACE_EVENT(fdb_delete,
> >>> __entry->addr[4], __entry->addr[5], __entry->vid)
> >>> );
> >>>
> >>> +TRACE_EVENT(br_fdb_update,
> >>> +
> >>> + TP_PROTO(struct net_bridge *br, struct net_bridge_port *source,
> >>> + const unsigned char *addr, u16 vid, bool added_by_user),
> >>> +
> >>> + TP_ARGS(br, source, addr, vid, added_by_user),
> >>> +
> >>> + TP_STRUCT__entry(
> >>> + __string(br_dev, br->dev->name)
> >>> + __string(dev, source->dev->name)
> >>
> >> I have found that using the device string name is
> >>
> >> (1) slow as it involves strcpy+strlen
> >>
> >> See [1]+[2] where a single dev-name costed me 16 ns, and the base
> >> overhead of a bpf attached tracepoint is 25 ns (see [3]).
> >>
> >> [1] https://git.kernel.org/davem/net-next/c/e7d12ce121a
> >> [2] https://git.kernel.org/davem/net-next/c/315ec3990ef
> >> [3] https://git.kernel.org/davem/net-next/c/25d4dae1a64
> >>
> >> (2) strings are also harder to work-with/extract when attaching a bpf_prog
> >>
> >> See the trouble I'm in accessing a dev string here napi:napi_poll here:
> >> https://github.com/netoptimizer/prototype-kernel/blob/103b955a080/kernel/samples/bpf/napi_monitor_kern.c#L52-L58
> >>
> >> Using ifindex'es in userspace is fairly easy see man if_indextoname(3).
> >>
> >
> > Jesper thanks for the data!. GTK. Looking at include/trace/events,
> > currently almost all tracepoints use dev->name.
True, but with my recent experience and benchmarking, I consider this
generally a bad choice we have made for all these tracepoints. In your
case with 2 strings, 2x16=32ns, you basically introduced a overhead
that is larger that to invocation cost.
> > These bridge tracepoints in context are primarily for debugging fdb
> > updates only, not for every packet and hence not in the performance
> > path.
> > In large scale deployments with thousands of bridge ports and fdb
> > entries, dev->name will definately make it easier to trouble-shoot.
> > So, I did like to leave these with dev->name unless there are strong objections.
>
> +1 for user friendliness for debugging tracepoints. The device name is
> also more user friendly when adding filters to the data collection.
>
> Being able to add bpf everywhere certainly changes the game a bit, but
> we should not relinquish ease of use and understanding for the potential
> that someone might want to put a bpf program on the tracepoint and want
> to maintain high performance.
(Cc. Acme and Peterz)
I wonder if we can create a special perf-tracepoint type for ifindex'es
and the tool reading (e.g. perf-script) can perform the name lookup in
userspace (calling if_indextoname(3)) ?
I don't know the perf tools well enough to know if this is possible?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH net] ipv4: Don't override return code from ip_route_input_noref()
From: Stefano Brivio @ 2017-08-31 16:11 UTC (permalink / raw)
To: David S . Miller, netdev; +Cc: Sabrina Dubroca, Wei Wang
After ip_route_input() calls ip_route_input_noref(), another
check on skb_dst() is done, but if this fails, we shouldn't
override the return code from ip_route_input_noref(), as it
could have been more specific (i.e. -EHOSTUNREACH).
This also saves one call to skb_dst_force_safe() and one to
skb_dst() in case the ip_route_input_noref() check fails.
Reported-by: Sabrina Dubroca <sdubroca@redhat.com>
Fixes: ad65a2f05695 ("ipv4: call dst_hold_safe() properly")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
include/net/route.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/net/route.h b/include/net/route.h
index cb0a76d9dde1..1b09a9368c68 100644
--- a/include/net/route.h
+++ b/include/net/route.h
@@ -189,10 +189,11 @@ static inline int ip_route_input(struct sk_buff *skb, __be32 dst, __be32 src,
rcu_read_lock();
err = ip_route_input_noref(skb, dst, src, tos, devin);
- if (!err)
+ if (!err) {
skb_dst_force_safe(skb);
- if (!skb_dst(skb))
- err = -EINVAL;
+ if (!skb_dst(skb))
+ err = -EINVAL;
+ }
rcu_read_unlock();
return err;
--
2.9.4
^ permalink raw reply related
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