* Re: [PATCH 2/2] at803x: double check SGMII side autoneg
From: Timur Tabi @ 2017-06-01 14:48 UTC (permalink / raw)
To: Zefir Kurtisi, Matthias May, Andrew Lunn
Cc: netdev, f.fainelli, David Miller, Manoj Iyer, jhugo
In-Reply-To: <265ee4d4-ce24-9e0f-d7c5-fba4ed36ab96@neratec.com>
On 06/01/2017 06:45 AM, Zefir Kurtisi wrote:
> I guess we need to decide whether we generally need to handle permanent aneg
> failures on the SGMII link. If we expect that it must not fail (like we assumed
> until we saw it failing), I agree with Timur and support reverting of the related
> commit f62265b53e. If otherwise we want this potential failure to be handled
> correctly, things become arbitrary complex. Essentially, we need to handle such
> PHYs as a combination of their two sides (copper + SGMII) as virtual sub-PHYs. The
> phylib might support that in a future version, but for now this seems like a lot
> of work required to handle a rare problem.
I'm about to post a patch that removes interrupt support from the EMAC
driver and relies on software polling of the PHY. With this patch, we
don't see the "link is not okay" message from that at803x driver any more.
The link state is generally more reliable now, even when the at803x
driver doesn't complain.
My theory is that the hardware polling of the PHY is just too
aggressive. I think it continuously reads the PHY status register at
maximum speed and immediately issues an interrupt when the PHY says that
it's up.
So I think we're okay with leaving the at803x driver as-is, since we
appear to be no longer getting any false failures.
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: Oops with commit 6d18c73 bridge: start hello_timer when enabling KERNEL_STP in br_stp_start
From: Nikolay Aleksandrov @ 2017-06-01 14:45 UTC (permalink / raw)
To: Sebastian Ott, Xin Long
Cc: David S. Miller, Haidong Li, Ivan Vecera, Stephen Hemminger,
network dev, LKML, Heiko Carstens, Martin Schwidefsky
In-Reply-To: <edd9b7f0-0345-975e-dc97-d5261134c7b4@cumulusnetworks.com>
On 01/06/17 17:16, Nikolay Aleksandrov wrote:
> On 01/06/17 17:00, Nikolay Aleksandrov wrote:
>> On 01/06/17 15:34, Sebastian Ott wrote:
>>> On Thu, 1 Jun 2017, Xin Long wrote:
>>>> On Thu, Jun 1, 2017 at 12:32 AM, Sebastian Ott
>>>> <sebott@linux.vnet.ibm.com> wrote:
>>>>> [...]
>>>> I couldn't see any bridge-related thing here, and it couldn't be reproduced
>>>> with virbr0 (stp=1) on my box (on both s390x and x86_64), I guess there
>>>> is something else in you machine.
>>>>
>>>> With the latest upstream kernel, can you remove libvirt (virbr0) and boot your
>>>> machine normally, then:
>>>> # brctl addbr br0
>>>> # ip link set br0 up
>>>> # brctl stp br0 on
>>>>
>>>> to check if it will still hang.
>>>
>>> Nope. That doesn't hang.
>>>
>>>
>>>> If it can't be reproduced in this way, pls add this on your kernel:
>>>>
>>>> --- a/net/bridge/br_stp_if.c
>>>> +++ b/net/bridge/br_stp_if.c
>>>> @@ -178,9 +178,11 @@ static void br_stp_start(struct net_bridge *br)
>>>> br->stp_enabled = BR_KERNEL_STP;
>>>> br_debug(br, "using kernel STP\n");
>>>>
>>>> + WARN_ON(1);
>>>> /* To start timers on any ports left in blocking */
>>>> mod_timer(&br->hello_timer, jiffies + br->hello_time);
>>>> br_port_state_selection(br);
>>>> + pr_warn("hello timer start done\n");
>>>> }
>>>>
>>>> spin_unlock_bh(&br->lock);
>>>> diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
>>>> index 60b6fe2..c98b3e5 100644
>>>> --- a/net/bridge/br_stp_timer.c
>>>> +++ b/net/bridge/br_stp_timer.c
>>>> @@ -40,7 +40,7 @@ static void br_hello_timer_expired(unsigned long arg)
>>>> if (br->dev->flags & IFF_UP) {
>>>> br_config_bpdu_generation(br);
>>>>
>>>> - if (br->stp_enabled == BR_KERNEL_STP)
>>>> + if (br->stp_enabled != BR_USER_STP)
>>>> mod_timer(&br->hello_timer,
>>>> round_jiffies(jiffies + br->hello_time));
>>>>
>>>>
>>>> let's see if it hangs when starting the timer. Thanks.
>>>
>>> No hang either:
>>>
>> [snip]
>> Could you please try the patch below ?
>>
>> ---
>>
>> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
>> index 4efd5d54498a..89110319ef0f 100644
>> --- a/net/bridge/br_stp_if.c
>> +++ b/net/bridge/br_stp_if.c
>> @@ -173,7 +173,8 @@ static void br_stp_start(struct net_bridge *br)
>> br_debug(br, "using kernel STP\n");
>>
>> /* To start timers on any ports left in blocking */
>> - mod_timer(&br->hello_timer, jiffies + br->hello_time);
>> + if (br->dev->flags & IFF_UP)
>> + mod_timer(&br->hello_timer, jiffies + br->hello_time);
>> br_port_state_selection(br);
>> }
>>
>>
>
> Ah nevermind, this patch reverts it back to the previous state.
>
Okay, I saw the problem and can reliably reproduce it. I will send a fix for testing
in a few minutes. I think the issue is that the timer can be started before the bridge
even goes up, i.e. create bridge -> brctl stp br0 on -> ip l del br0
so the del_timer_sync() doesn't get executed and thus it's still armed.
$ while :; do ip l add br0 type bridge hello_time 100; brctl stp br0 on; ip l del br0; done;
^ permalink raw reply
* Re: commit f5f99309 (sock: do not set sk_err in sock_dequeue_err_skb) has broken ping
From: Soheil Hassas Yeganeh @ 2017-06-01 14:39 UTC (permalink / raw)
To: Cyril Hrubis
Cc: David S. Miller, Shmulik Ladkani, Marcelo Ricardo Leitner,
Pravin B Shelar, Eric Dumazet, WANG Cong, Yaogong Wang,
Steffen Klassert, Al Viro, netdev, linux-kernel
In-Reply-To: <20170601143141.GB24401@rei.lan>
On Thu, Jun 1, 2017 at 10:31 AM, Cyril Hrubis <chrubis@suse.cz> wrote:
> I've started bisecting on v4.11 and see the problem on v4.10 on another
> machine, the patch should be there in both cases and the bug is easily
> reproducible.
Thank you for the confirmation. Could you please try the following
patch to see if it fixes your issue?
>From 3ec438460425d127741b20f03f78644c9e441e8c Mon Sep 17 00:00:00 2001
From: Soheil Hassas Yeganeh <soheil@google.com>
Date: Thu, 1 Jun 2017 10:34:09 -0400
Subject: [PATCH net] sock: reset sk_err when the error queue is empty
Before f5f99309fa74 (sock: do not set sk_err in
sock_dequeue_err_skb), sk_err was reset to 0 upon reading
from the error queue when the error queue was empty.
Applications, most notably ping, are relying on this
behavior to reset sk_err.
Reset sk_err when there is no packet left on the
error queue.
Fixes: f5f99309fa74 (sock: do not set sk_err in sock_dequeue_err_skb)
Reported-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
---
net/core/skbuff.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 346d3e85dfbc..5a726161f4e4 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3758,7 +3758,7 @@ struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
icmp_next = is_icmp_err_skb(skb_next);
spin_unlock_irqrestore(&q->lock, flags);
- if (is_icmp_err_skb(skb) && !icmp_next)
+ if ((is_icmp_err_skb(skb) && !icmp_next) || !skb_next)
sk->sk_err = 0;
if (skb_next)
--
2.13.0.219.gdb65acc882-goog
>> 2. I've also have sent a fix to iputils on
>> https://github.com/iputils/iputils/pull/75. Would you be kind to try
>> that pull request as well?
>
> That fixed the problem, you can add:
>
> Tested-by: Cyril Hrubis <chrubis@suse.cz>
Thank you for testing! Will do.
^ permalink raw reply related
* Re: commit f5f99309 (sock: do not set sk_err in sock_dequeue_err_skb) has broken ping
From: Cyril Hrubis @ 2017-06-01 14:31 UTC (permalink / raw)
To: Soheil Hassas Yeganeh
Cc: David S. Miller, Shmulik Ladkani, Marcelo Ricardo Leitner,
Pravin B Shelar, Eric Dumazet, WANG Cong, Yaogong Wang,
Steffen Klassert, Al Viro, netdev, linux-kernel
In-Reply-To: <CACSApvZiQTYSVkS62YdadjabUTqwO9O2=Sq4ELuO3eoJ_LM_ZQ@mail.gmail.com>
Hi!
> > I've bisected the problem to this commit:
> >
> > commit f5f99309fa7481f59a500f0d08f3379cd6424c1f (HEAD, refs/bisect/bad)
> > Author: Soheil Hassas Yeganeh <soheil@google.com>
> > Date: Thu Nov 3 18:24:27 2016 -0400
> >
> > sock: do not set sk_err in sock_dequeue_err_skb
>
> Hi Cyril,
>
> I'm sorry for the problem, and thank you for the report.
>
> Two questions:
> 1. Could you double check whether you have the following commit in your tree?
>
> commit 83a1a1a70e87f676fbb6086b26b6ac7f7fdd107d
> Author: Soheil Hassas Yeganeh <soheil@google.com>
> Date: Wed Nov 30 14:01:08 2016 -0500
> sock: reset sk_err for ICMP packets read from error queue
I've started bisecting on v4.11 and see the problem on v4.10 on another
machine, the patch should be there in both cases and the bug is easily
reproducible.
> 2. I've also have sent a fix to iputils on
> https://github.com/iputils/iputils/pull/75. Would you be kind to try
> that pull request as well?
That fixed the problem, you can add:
Tested-by: Cyril Hrubis <chrubis@suse.cz>
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply
* Re: Oops with commit 6d18c73 bridge: start hello_timer when enabling KERNEL_STP in br_stp_start
From: Nikolay Aleksandrov @ 2017-06-01 14:16 UTC (permalink / raw)
To: Sebastian Ott, Xin Long
Cc: David S. Miller, Haidong Li, Ivan Vecera, Stephen Hemminger,
network dev, LKML, Heiko Carstens, Martin Schwidefsky
In-Reply-To: <a6495939-350d-31a4-88db-852344a19a02@cumulusnetworks.com>
On 01/06/17 17:00, Nikolay Aleksandrov wrote:
> On 01/06/17 15:34, Sebastian Ott wrote:
>> On Thu, 1 Jun 2017, Xin Long wrote:
>>> On Thu, Jun 1, 2017 at 12:32 AM, Sebastian Ott
>>> <sebott@linux.vnet.ibm.com> wrote:
>>>> [...]
>>> I couldn't see any bridge-related thing here, and it couldn't be reproduced
>>> with virbr0 (stp=1) on my box (on both s390x and x86_64), I guess there
>>> is something else in you machine.
>>>
>>> With the latest upstream kernel, can you remove libvirt (virbr0) and boot your
>>> machine normally, then:
>>> # brctl addbr br0
>>> # ip link set br0 up
>>> # brctl stp br0 on
>>>
>>> to check if it will still hang.
>>
>> Nope. That doesn't hang.
>>
>>
>>> If it can't be reproduced in this way, pls add this on your kernel:
>>>
>>> --- a/net/bridge/br_stp_if.c
>>> +++ b/net/bridge/br_stp_if.c
>>> @@ -178,9 +178,11 @@ static void br_stp_start(struct net_bridge *br)
>>> br->stp_enabled = BR_KERNEL_STP;
>>> br_debug(br, "using kernel STP\n");
>>>
>>> + WARN_ON(1);
>>> /* To start timers on any ports left in blocking */
>>> mod_timer(&br->hello_timer, jiffies + br->hello_time);
>>> br_port_state_selection(br);
>>> + pr_warn("hello timer start done\n");
>>> }
>>>
>>> spin_unlock_bh(&br->lock);
>>> diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
>>> index 60b6fe2..c98b3e5 100644
>>> --- a/net/bridge/br_stp_timer.c
>>> +++ b/net/bridge/br_stp_timer.c
>>> @@ -40,7 +40,7 @@ static void br_hello_timer_expired(unsigned long arg)
>>> if (br->dev->flags & IFF_UP) {
>>> br_config_bpdu_generation(br);
>>>
>>> - if (br->stp_enabled == BR_KERNEL_STP)
>>> + if (br->stp_enabled != BR_USER_STP)
>>> mod_timer(&br->hello_timer,
>>> round_jiffies(jiffies + br->hello_time));
>>>
>>>
>>> let's see if it hangs when starting the timer. Thanks.
>>
>> No hang either:
>>
> [snip]
> Could you please try the patch below ?
>
> ---
>
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index 4efd5d54498a..89110319ef0f 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -173,7 +173,8 @@ static void br_stp_start(struct net_bridge *br)
> br_debug(br, "using kernel STP\n");
>
> /* To start timers on any ports left in blocking */
> - mod_timer(&br->hello_timer, jiffies + br->hello_time);
> + if (br->dev->flags & IFF_UP)
> + mod_timer(&br->hello_timer, jiffies + br->hello_time);
> br_port_state_selection(br);
> }
>
>
Ah nevermind, this patch reverts it back to the previous state.
^ permalink raw reply
* Re: commit f5f99309 (sock: do not set sk_err in sock_dequeue_err_skb) has broken ping
From: Soheil Hassas Yeganeh @ 2017-06-01 14:10 UTC (permalink / raw)
To: Cyril Hrubis
Cc: David S. Miller, Shmulik Ladkani, Marcelo Ricardo Leitner,
Pravin B Shelar, Eric Dumazet, WANG Cong, Yaogong Wang,
Steffen Klassert, Al Viro, netdev, linux-kernel
In-Reply-To: <20170601140048.GA24401@rei.lan>
On Thu, Jun 1, 2017 at 10:00 AM, Cyril Hrubis <chrubis@suse.cz> wrote:
> I've bisected the problem to this commit:
>
> commit f5f99309fa7481f59a500f0d08f3379cd6424c1f (HEAD, refs/bisect/bad)
> Author: Soheil Hassas Yeganeh <soheil@google.com>
> Date: Thu Nov 3 18:24:27 2016 -0400
>
> sock: do not set sk_err in sock_dequeue_err_skb
Hi Cyril,
I'm sorry for the problem, and thank you for the report.
Two questions:
1. Could you double check whether you have the following commit in your tree?
commit 83a1a1a70e87f676fbb6086b26b6ac7f7fdd107d
Author: Soheil Hassas Yeganeh <soheil@google.com>
Date: Wed Nov 30 14:01:08 2016 -0500
sock: reset sk_err for ICMP packets read from error queue
2. I've also have sent a fix to iputils on
https://github.com/iputils/iputils/pull/75. Would you be kind to try
that pull request as well?
Thanks,
Soheil
^ permalink raw reply
* Re: [PATCH v3] net: don't call strlen on non-terminated string in dev_set_alias()
From: Alexander Potapenko @ 2017-06-01 14:06 UTC (permalink / raw)
To: Yury Norov
Cc: Dmitriy Vyukov, Kostya Serebryany, Eric Dumazet, David Miller,
stephen, LKML, Networking
In-Reply-To: <20170601140453.fp7iusjomq65tu6i@yury-thinkpad>
On Thu, Jun 1, 2017 at 4:04 PM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> On Thu, Jun 01, 2017 at 03:50:33PM +0200, Alexander Potapenko wrote:
>> On Thu, Jun 1, 2017 at 3:47 PM, Yury Norov <ynorov@caviumnetworks.com> wrote:
>> > On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote:
>> >> KMSAN reported a use of uninitialized memory in dev_set_alias(),
>> >> which was caused by calling strlcpy() (which in turn called strlen())
>> >> on the user-supplied non-terminated string.
>> >>
>> >> Signed-off-by: Alexander Potapenko <glider@google.com>
>> >> ---
>> >> v3: removed the multi-line comment
>> >> v2: fixed an off-by-one error spotted by Dmitry Vyukov
>> >
>> > [...]
>> >
>> >> ---
>> >> net/core/dev.c | 4 +++-
>> >> 1 file changed, 3 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/core/dev.c b/net/core/dev.c
>> >> index fca407b4a6ea..3e3b29133cc9 100644
>> >> --- a/net/core/dev.c
>> >> +++ b/net/core/dev.c
>> >> @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
>> >> return -ENOMEM;
>> >> dev->ifalias = new_ifalias;
>> >>
>> >> - strlcpy(dev->ifalias, alias, len+1);
>> >> + /* alias comes from the userspace and may not be zero-terminated. */
>> >
>> > So if the comment is correct, you'd use copy_from_user() instead.
>> Well, the contents of |alias| have been previously copied from the
>> userspace, but this is a pointer to a kernel buffer, as the function
>> prototype tells.
>> Do you think a confusion is possible here?
>
> Yes, I think so... If pointer comes from userspace, it normally points to
> userspace data. If you have the data already copied, just say:
> + /* alias may not be zero-terminated. */
>
> Or say nothing, because at the first glance, using the strlcpy()
> instead of simple memcpy() looks weird in this case - you know the
> length of the string from the beginning, and should not recalculate it
> again.
You're right, I'll just drop this line.
> Yury
>
>> >> + memcpy(dev->ifalias, alias, len);
>> >> + dev->ifalias[len] = 0;
>> >> return len;
>> >> }
>> >>
>> >> --
>> >> 2.13.0.219.gdb65acc882-goog
>>
>>
>>
>> --
>> Alexander Potapenko
>> Software Engineer
>>
>> Google Germany GmbH
>> Erika-Mann-Straße, 33
>> 80636 München
>>
>> Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
>> Registergericht und -nummer: Hamburg, HRB 86891
>> Sitz der Gesellschaft: Hamburg
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH v3] net: don't call strlen on non-terminated string in dev_set_alias()
From: Yury Norov @ 2017-06-01 14:04 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Dmitriy Vyukov, Kostya Serebryany, Eric Dumazet, David Miller,
stephen, LKML, Networking
In-Reply-To: <CAG_fn=U0n4Sn4_44e63FjsYPTTDX4ivAbTcuL2s3erFc+g6n2g@mail.gmail.com>
On Thu, Jun 01, 2017 at 03:50:33PM +0200, Alexander Potapenko wrote:
> On Thu, Jun 1, 2017 at 3:47 PM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> > On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote:
> >> KMSAN reported a use of uninitialized memory in dev_set_alias(),
> >> which was caused by calling strlcpy() (which in turn called strlen())
> >> on the user-supplied non-terminated string.
> >>
> >> Signed-off-by: Alexander Potapenko <glider@google.com>
> >> ---
> >> v3: removed the multi-line comment
> >> v2: fixed an off-by-one error spotted by Dmitry Vyukov
> >
> > [...]
> >
> >> ---
> >> net/core/dev.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index fca407b4a6ea..3e3b29133cc9 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
> >> return -ENOMEM;
> >> dev->ifalias = new_ifalias;
> >>
> >> - strlcpy(dev->ifalias, alias, len+1);
> >> + /* alias comes from the userspace and may not be zero-terminated. */
> >
> > So if the comment is correct, you'd use copy_from_user() instead.
> Well, the contents of |alias| have been previously copied from the
> userspace, but this is a pointer to a kernel buffer, as the function
> prototype tells.
> Do you think a confusion is possible here?
Yes, I think so... If pointer comes from userspace, it normally points to
userspace data. If you have the data already copied, just say:
+ /* alias may not be zero-terminated. */
Or say nothing, because at the first glance, using the strlcpy()
instead of simple memcpy() looks weird in this case - you know the
length of the string from the beginning, and should not recalculate it
again.
Yury
> >> + memcpy(dev->ifalias, alias, len);
> >> + dev->ifalias[len] = 0;
> >> return len;
> >> }
> >>
> >> --
> >> 2.13.0.219.gdb65acc882-goog
>
>
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: Oops with commit 6d18c73 bridge: start hello_timer when enabling KERNEL_STP in br_stp_start
From: Nikolay Aleksandrov @ 2017-06-01 14:00 UTC (permalink / raw)
To: Sebastian Ott, Xin Long
Cc: David S. Miller, Haidong Li, Ivan Vecera, Stephen Hemminger,
network dev, LKML, Heiko Carstens, Martin Schwidefsky
In-Reply-To: <alpine.LFD.2.20.1706011415500.1688@schleppi>
On 01/06/17 15:34, Sebastian Ott wrote:
> On Thu, 1 Jun 2017, Xin Long wrote:
>> On Thu, Jun 1, 2017 at 12:32 AM, Sebastian Ott
>> <sebott@linux.vnet.ibm.com> wrote:
>>> [...]
>> I couldn't see any bridge-related thing here, and it couldn't be reproduced
>> with virbr0 (stp=1) on my box (on both s390x and x86_64), I guess there
>> is something else in you machine.
>>
>> With the latest upstream kernel, can you remove libvirt (virbr0) and boot your
>> machine normally, then:
>> # brctl addbr br0
>> # ip link set br0 up
>> # brctl stp br0 on
>>
>> to check if it will still hang.
>
> Nope. That doesn't hang.
>
>
>> If it can't be reproduced in this way, pls add this on your kernel:
>>
>> --- a/net/bridge/br_stp_if.c
>> +++ b/net/bridge/br_stp_if.c
>> @@ -178,9 +178,11 @@ static void br_stp_start(struct net_bridge *br)
>> br->stp_enabled = BR_KERNEL_STP;
>> br_debug(br, "using kernel STP\n");
>>
>> + WARN_ON(1);
>> /* To start timers on any ports left in blocking */
>> mod_timer(&br->hello_timer, jiffies + br->hello_time);
>> br_port_state_selection(br);
>> + pr_warn("hello timer start done\n");
>> }
>>
>> spin_unlock_bh(&br->lock);
>> diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
>> index 60b6fe2..c98b3e5 100644
>> --- a/net/bridge/br_stp_timer.c
>> +++ b/net/bridge/br_stp_timer.c
>> @@ -40,7 +40,7 @@ static void br_hello_timer_expired(unsigned long arg)
>> if (br->dev->flags & IFF_UP) {
>> br_config_bpdu_generation(br);
>>
>> - if (br->stp_enabled == BR_KERNEL_STP)
>> + if (br->stp_enabled != BR_USER_STP)
>> mod_timer(&br->hello_timer,
>> round_jiffies(jiffies + br->hello_time));
>>
>>
>> let's see if it hangs when starting the timer. Thanks.
>
> No hang either:
>
[snip]
Could you please try the patch below ?
---
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 4efd5d54498a..89110319ef0f 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -173,7 +173,8 @@ static void br_stp_start(struct net_bridge *br)
br_debug(br, "using kernel STP\n");
/* To start timers on any ports left in blocking */
- mod_timer(&br->hello_timer, jiffies + br->hello_time);
+ if (br->dev->flags & IFF_UP)
+ mod_timer(&br->hello_timer, jiffies + br->hello_time);
br_port_state_selection(br);
}
^ permalink raw reply related
* commit f5f99309 (sock: do not set sk_err in sock_dequeue_err_skb) has broken ping
From: Cyril Hrubis @ 2017-06-01 14:00 UTC (permalink / raw)
To: David S. Miller, Shmulik Ladkani, Marcelo Ricardo Leitner,
Pravin B Shelar, Eric Dumazet, Soheil Hassas Yeganeh, WANG Cong,
Yaogong Wang, Steffen Klassert, Al Viro, netdev, linux-kernel
Hi!
I've started to wonder why is ping eating 100% CPU shortly after I've
upgraded my machine to 4.10 and here is what I found:
The ping main_loop() sleeps in poll() on its socket, the poll() usually times
out, at least that's what strace suggets which causes ping to sleep for ~1s in
the kernel.
See ping source at:
https://github.com/iputils/iputils/blob/master/ping_common.c#L587
The poll() seems to start returning POLLERR immediatelly after poll() is called
on the socket in a case that connection has dropped for a short while. It seems to be easily reproducible with:
* Starting ping with some ip address i.e. ping 4.2.2.2
* Letting it ping for a minute or so
* Disconnection a WAN cable from your AP
* After a minute or so ping ends up bussy looping on
poll() that returns with POLLERR immediatelly
* After plugging the cable back the problem gets only
worse since we now spend 99% of the time bussy looping
on the poll() syscall
* And my CPU fan starts to scream loudly
I've bisected the problem to this commit:
commit f5f99309fa7481f59a500f0d08f3379cd6424c1f (HEAD, refs/bisect/bad)
Author: Soheil Hassas Yeganeh <soheil@google.com>
Date: Thu Nov 3 18:24:27 2016 -0400
sock: do not set sk_err in sock_dequeue_err_skb
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply
* Re: [PATCH v2 net-next 0/8] Introduce bpf ID
From: David Ahern @ 2017-06-01 13:55 UTC (permalink / raw)
To: Jakub Kicinski, Martin KaFai Lau, Hannes Frederic Sowa
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team
In-Reply-To: <20170531192256.0e0d0ce1@cakuba.lan>
On 5/31/17 8:22 PM, Jakub Kicinski wrote:
> On Wed, 31 May 2017 11:58:54 -0700, Martin KaFai Lau wrote:
>> This patch series:
>> 1) Introduce ID for both bpf_prog and bpf_map.
>> 2) Add bpf commands to iterate the prog IDs and map
>> IDs of the system.
>> 3) Add bpf commands to get a prog/map fd from an ID
>> 4) Add bpf command to get prog/map info from a fd.
>> The prog/map info is a jump start in this patchset
>> and it is not meant to be a complete list. They can
>> be extended in the future patches.
>
> Hi! IIUC there were plans at some point to store the BPF byte code in
> the kernel, the way it was loaded from the user space and before it
> went through the verifier. Are you still planning on implementing that?
> It would be handy for offloads to have the original byte code, things
> like call inlining make translations of maps calls a bit challenging,
> since we try to run through the verifier again..
>
Save the instructions:
https://github.com/dsahern/linux/commit/5315578db7b0886a3e8e3bef6a56e828ae2bd2c4
Let userspace read it back:
https://github.com/dsahern/linux/commit/619982064b9b582fc5d4a504feffca1730610c4e
One more in between in the branch:
https://github.com/dsahern/linux/tree/bpf/retrieve-bpf-wip
Now that the id code is in, it needs to be returned on link dumps, route
dumps, etc and accessible via fdinfo. The id can be turned into an fd
and the fd read using the above patches.
^ permalink raw reply
* RE: [PATCH][net-next] net: dsa: make function ksz_rcv static
From: Woojung.Huh @ 2017-06-01 13:53 UTC (permalink / raw)
To: colin.king, UNGLinuxDriver, andrew, vivien.didelot, f.fainelli,
davem, netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20170601122242.14341-1-colin.king@canonical.com>
> function ksz_rcv can be made static as it does not need to be
> in global scope. Reformat arguments to make it checkpatch warning
> free too.
>
> Cleans up sparse warning: "symbol 'ksz_rcv' was not declared. Should
> it be static?"
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reviewed-by: Woojung Huh <Woojung.Huh@microchip.com>
^ permalink raw reply
* Re: [PATCH v3] net: don't call strlen on non-terminated string in dev_set_alias()
From: Alexander Potapenko @ 2017-06-01 13:50 UTC (permalink / raw)
To: Yury Norov
Cc: Dmitriy Vyukov, Kostya Serebryany, Eric Dumazet, David Miller,
stephen, LKML, Networking
In-Reply-To: <20170601134737.7dp2pbnek26b6kqf@yury-thinkpad>
On Thu, Jun 1, 2017 at 3:47 PM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote:
>> KMSAN reported a use of uninitialized memory in dev_set_alias(),
>> which was caused by calling strlcpy() (which in turn called strlen())
>> on the user-supplied non-terminated string.
>>
>> Signed-off-by: Alexander Potapenko <glider@google.com>
>> ---
>> v3: removed the multi-line comment
>> v2: fixed an off-by-one error spotted by Dmitry Vyukov
>
> [...]
>
>> ---
>> net/core/dev.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index fca407b4a6ea..3e3b29133cc9 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
>> return -ENOMEM;
>> dev->ifalias = new_ifalias;
>>
>> - strlcpy(dev->ifalias, alias, len+1);
>> + /* alias comes from the userspace and may not be zero-terminated. */
>
> So if the comment is correct, you'd use copy_from_user() instead.
Well, the contents of |alias| have been previously copied from the
userspace, but this is a pointer to a kernel buffer, as the function
prototype tells.
Do you think a confusion is possible here?
>> + memcpy(dev->ifalias, alias, len);
>> + dev->ifalias[len] = 0;
>> return len;
>> }
>>
>> --
>> 2.13.0.219.gdb65acc882-goog
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH v3] net: don't call strlen on non-terminated string in dev_set_alias()
From: Yury Norov @ 2017-06-01 13:47 UTC (permalink / raw)
To: Alexander Potapenko
Cc: dvyukov, kcc, edumazet, davem, stephen, linux-kernel, netdev
In-Reply-To: <20170601123829.51794-1-glider@google.com>
On Thu, Jun 01, 2017 at 02:38:29PM +0200, Alexander Potapenko wrote:
> KMSAN reported a use of uninitialized memory in dev_set_alias(),
> which was caused by calling strlcpy() (which in turn called strlen())
> on the user-supplied non-terminated string.
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
> v3: removed the multi-line comment
> v2: fixed an off-by-one error spotted by Dmitry Vyukov
[...]
> ---
> net/core/dev.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fca407b4a6ea..3e3b29133cc9 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
> return -ENOMEM;
> dev->ifalias = new_ifalias;
>
> - strlcpy(dev->ifalias, alias, len+1);
> + /* alias comes from the userspace and may not be zero-terminated. */
So if the comment is correct, you'd use copy_from_user() instead.
> + memcpy(dev->ifalias, alias, len);
> + dev->ifalias[len] = 0;
> return len;
> }
>
> --
> 2.13.0.219.gdb65acc882-goog
^ permalink raw reply
* Re: [PATCH v2 net-next 1/3] perf, bpf: Add BPF support to all perf_event types
From: Peter Zijlstra @ 2017-06-01 13:32 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S . Miller, Brendan Gregg, Daniel Borkmann, Teng Qin,
netdev, linux-kernel
In-Reply-To: <20170530190339.svpp53bggfznc476@hirez.programming.kicks-ass.net>
On Tue, May 30, 2017 at 09:03:39PM +0200, Peter Zijlstra wrote:
> On Tue, May 30, 2017 at 10:37:46AM -0700, Alexei Starovoitov wrote:
> > On 5/30/17 9:51 AM, Peter Zijlstra wrote:
>
> > > I'm not entirely sure I see how that is required. Should per task not
> > > already work? The WARN that's there will only trigger if you call them
> > > on the wrong task, which is something you shouldn't do anyway.
> >
> > The kernel WARN is considered to be a bug of bpf infra. That's the
> > reason we do all these checks at map update time and at run-time.
> > The bpf program authors should be able to do all possible experiments
> > until their scripts work. Dealing with kernel warns and reboots is not
> > something user space folks like to do.
> > Today bpf_perf_event_read() for per-task events isn't really
> > working due to event->oncpu != cpu runtime check in there.
> > If we convert warns to returns the existing scripts will continue
> > to work as-is and per-task will be possible.
>
> Ah yes.. I always forget that side of things (not ever having seen a
> bpf thing working doesn't help either of course).
>
> Let me consider that for a bit..
OK, do that. Something like the below should do I suppose.
It will return -EOPNOTSUPP for permanent failure (it cannot ever work)
and -EINVAL for temporary failure (could work if you call it on another
task/cpu).
---
kernel/events/core.c | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 8d6acaeeea17..22b6407da374 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3637,10 +3637,10 @@ static inline u64 perf_event_count(struct perf_event *event)
* will not be local and we cannot read them atomically
* - must not have a pmu::count method
*/
-u64 perf_event_read_local(struct perf_event *event)
+int perf_event_read_local(struct perf_event *event, u64 *value)
{
unsigned long flags;
- u64 val;
+ int ret = 0;
/*
* Disabling interrupts avoids all counter scheduling (context
@@ -3648,25 +3648,37 @@ u64 perf_event_read_local(struct perf_event *event)
*/
local_irq_save(flags);
- /* If this is a per-task event, it must be for current */
- WARN_ON_ONCE((event->attach_state & PERF_ATTACH_TASK) &&
- event->hw.target != current);
-
- /* If this is a per-CPU event, it must be for this CPU */
- WARN_ON_ONCE(!(event->attach_state & PERF_ATTACH_TASK) &&
- event->cpu != smp_processor_id());
-
/*
* It must not be an event with inherit set, we cannot read
* all child counters from atomic context.
*/
- WARN_ON_ONCE(event->attr.inherit);
+ if (event->attr.inherit) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
/*
* It must not have a pmu::count method, those are not
* NMI safe.
*/
- WARN_ON_ONCE(event->pmu->count);
+ if (event->pmu->count) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
+
+ /* If this is a per-task event, it must be for current */
+ if ((event->attach_state & PERF_ATTACH_TASK) &&
+ event->hw.target != current) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ /* If this is a per-CPU event, it must be for this CPU */
+ if (!(event->attach_state & PERF_ATTACH_TASK) &&
+ event->cpu != smp_processor_id()) {
+ ret = -EINVAL;
+ goto out;
+ }
/*
* If the event is currently on this CPU, its either a per-task event,
@@ -3676,10 +3688,11 @@ u64 perf_event_read_local(struct perf_event *event)
if (event->oncpu == smp_processor_id())
event->pmu->read(event);
- val = local64_read(&event->count);
+ *value = local64_read(&event->count);
+out:
local_irq_restore(flags);
- return val;
+ return ret;
}
static int perf_event_read(struct perf_event *event, bool group)
^ permalink raw reply related
* [PATCH net-next] mlxsw: spectrum: Implement the ethtool flash_device callback
From: Yotam Gigi @ 2017-06-01 13:26 UTC (permalink / raw)
To: netdev, idosch, eladr, ogerlitz, ilant, jiri, arkadis; +Cc: Yotam Gigi
Add callback to the ethtool flash_device op. This callback uses the mlxfw
module to flash the new firmware file to the device.
As the firmware flash process takes about 20 seconds and ethtool takes the
rtnl lock during the flash_device callback, release the rtnl lock at the
beginning of the flash process and take it again before leaving the
callback. This way, the rtnl is not held during the process. To make sure
the device does not get deleted during the flash process, take a reference
to it before releasing the rtnl lock.
Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 51 +++++++++++++++++++++-----
1 file changed, 42 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index e25c1fd..84b6f36 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -321,6 +321,21 @@ static const struct mlxfw_dev_ops mlxsw_sp_mlxfw_dev_ops = {
.fsm_release = mlxsw_sp_fsm_release
};
+static int mlxsw_sp_firmware_flash(struct mlxsw_sp *mlxsw_sp,
+ const struct firmware *firmware)
+{
+ struct mlxsw_sp_mlxfw_dev mlxsw_sp_mlxfw_dev = {
+ .mlxfw_dev = {
+ .ops = &mlxsw_sp_mlxfw_dev_ops,
+ .psid = mlxsw_sp->bus_info->psid,
+ .psid_size = strlen(mlxsw_sp->bus_info->psid),
+ },
+ .mlxsw_sp = mlxsw_sp
+ };
+
+ return mlxfw_firmware_flash(&mlxsw_sp_mlxfw_dev.mlxfw_dev, firmware);
+}
+
static bool mlxsw_sp_fw_rev_ge(const struct mlxsw_fw_rev *a,
const struct mlxsw_fw_rev *b)
{
@@ -334,14 +349,6 @@ static bool mlxsw_sp_fw_rev_ge(const struct mlxsw_fw_rev *a,
static int mlxsw_sp_fw_rev_validate(struct mlxsw_sp *mlxsw_sp)
{
const struct mlxsw_fw_rev *rev = &mlxsw_sp->bus_info->fw_rev;
- struct mlxsw_sp_mlxfw_dev mlxsw_sp_mlxfw_dev = {
- .mlxfw_dev = {
- .ops = &mlxsw_sp_mlxfw_dev_ops,
- .psid = mlxsw_sp->bus_info->psid,
- .psid_size = strlen(mlxsw_sp->bus_info->psid),
- },
- .mlxsw_sp = mlxsw_sp
- };
const struct firmware *firmware;
int err;
@@ -361,7 +368,7 @@ static int mlxsw_sp_fw_rev_validate(struct mlxsw_sp *mlxsw_sp)
return err;
}
- err = mlxfw_firmware_flash(&mlxsw_sp_mlxfw_dev.mlxfw_dev, firmware);
+ err = mlxsw_sp_firmware_flash(mlxsw_sp, firmware);
release_firmware(firmware);
return err;
}
@@ -2495,6 +2502,31 @@ mlxsw_sp_port_set_link_ksettings(struct net_device *dev,
return 0;
}
+static int mlxsw_sp_flash_device(struct net_device *dev,
+ struct ethtool_flash *flash)
+{
+ struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
+ struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port->mlxsw_sp;
+ const struct firmware *firmware;
+ int err;
+
+ if (flash->region != ETHTOOL_FLASH_ALL_REGIONS)
+ return -EOPNOTSUPP;
+
+ dev_hold(dev);
+ rtnl_unlock();
+
+ err = request_firmware_direct(&firmware, flash->data, &dev->dev);
+ if (err)
+ goto out;
+ err = mlxsw_sp_firmware_flash(mlxsw_sp, firmware);
+ release_firmware(firmware);
+out:
+ rtnl_lock();
+ dev_put(dev);
+ return err;
+}
+
static const struct ethtool_ops mlxsw_sp_port_ethtool_ops = {
.get_drvinfo = mlxsw_sp_port_get_drvinfo,
.get_link = ethtool_op_get_link,
@@ -2506,6 +2538,7 @@ static const struct ethtool_ops mlxsw_sp_port_ethtool_ops = {
.get_sset_count = mlxsw_sp_port_get_sset_count,
.get_link_ksettings = mlxsw_sp_port_get_link_ksettings,
.set_link_ksettings = mlxsw_sp_port_set_link_ksettings,
+ .flash_device = mlxsw_sp_flash_device,
};
static int
--
2.8.4
^ permalink raw reply related
* Re: [PATCH 2/5] net: phy: hook up clause 45 autonegotiation restart
From: Andrew Lunn @ 2017-06-01 13:19 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Florian Fainelli, netdev
In-Reply-To: <20170601130900.GA22219@n2100.armlinux.org.uk>
On Thu, Jun 01, 2017 at 02:09:00PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 01, 2017 at 03:05:27PM +0200, Andrew Lunn wrote:
> > So you are saying a 10G PHY driver always needs to have a aneg_done
> > callback, even if it just needs to call phygen_c45_aneg_done?
> >
> > This seems a bit error prone. I can see somebody writing a 10G driver,
> > leaving out aneg_done() and having the c22 version called. Is the read
> > of MII_BMSR likely to return 0xffff, since the register does not
> > exist? If so, genphy_aneg_done() is likely to always return
> > BMSR_ANEGCOMPLETE.
>
> Don't forget that the read will fail, so phy_read() will return a
> negative number.
By fail, you mean return something like -EIO or -ETIMEOUT? Is this
guaranteed in the code somewhere? This particular Marvell PHY only
does c45. But i could imagine some other PHYs answering a c22 request
with 0xffff.
Andrew
^ permalink raw reply
* Re: [PATCH 2/5] net: phy: hook up clause 45 autonegotiation restart
From: Russell King - ARM Linux @ 2017-06-01 13:09 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, netdev
In-Reply-To: <20170601130527.GG9282@lunn.ch>
On Thu, Jun 01, 2017 at 03:05:27PM +0200, Andrew Lunn wrote:
> So you are saying a 10G PHY driver always needs to have a aneg_done
> callback, even if it just needs to call phygen_c45_aneg_done?
>
> This seems a bit error prone. I can see somebody writing a 10G driver,
> leaving out aneg_done() and having the c22 version called. Is the read
> of MII_BMSR likely to return 0xffff, since the register does not
> exist? If so, genphy_aneg_done() is likely to always return
> BMSR_ANEGCOMPLETE.
Don't forget that the read will fail, so phy_read() will return a
negative number. This practically (and I've tested it) means that
genphy_aneg_done() never indicates negotiation completion, and the
link never comes up.
> Seems like a trap waiting for somebody to fall into it. The additional
> code might be worth it to avoid placing this trap.
The "trap" means a non-functional driver, since the link doesn't
come up.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 5/5] net: phy: add Marvell Alaska X 88X3310 10Gigabit PHY support
From: Russell King - ARM Linux @ 2017-06-01 13:06 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, netdev
In-Reply-To: <20170601125150.GE9282@lunn.ch>
On Thu, Jun 01, 2017 at 02:51:50PM +0200, Andrew Lunn wrote:
> > +static int mv3310_read_status(struct phy_device *phydev)
> > +{
> > + u32 mmd_mask = phydev->c45_ids.devices_in_package;
> > + int val;
> > +
> > + /* The vendor devads do not report link status. Avoid the PHYXS
> > + * instance as there are three, and its status depends on the MAC
> > + * being appropriately configured for the negotiated speed.
> > + */
> > + mmd_mask &= ~(BIT(MDIO_MMD_VEND1) | BIT(MDIO_MMD_VEND2) |
> > + BIT(MDIO_MMD_PHYXS));
> > +
> > + phydev->speed = SPEED_UNKNOWN;
> > + phydev->duplex = DUPLEX_UNKNOWN;
> > + phydev->lp_advertising = 0;
> > + phydev->link = 0;
> > + phydev->pause = 0;
> > + phydev->asym_pause = 0;
> > +
> > + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_STAT1);
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & MDIO_STAT1_LSTATUS)
> > + return mv3310_read_10gbr_status(phydev);
> > +
> > + val = genphy_c45_read_link(phydev, mmd_mask);
> > + if (val < 0)
> > + return val;
> > +
> > + phydev->link = val > 0 ? 1 : 0;
> > +
> > + val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & MDIO_AN_STAT1_COMPLETE) {
> > + val = genphy_c45_read_lpa(phydev);
> > + if (val < 0)
> > + return val;
> > +
> > + /* Read the link partner's 1G advertisment */
> > + val = phy_read_mmd(phydev, MDIO_MMD_AN, MV_AN_STAT1000);
> > + if (val < 0)
> > + return val;
> > +
> > + phydev->lp_advertising |= mii_stat1000_to_ethtool_lpa_t(val);
> > +
> > + if (phydev->autoneg == AUTONEG_ENABLE) {
> > + val = phy_read_mmd(phydev, MDIO_MMD_AN, MV_AN_RESULT);
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & MV_AN_RESULT_SPD_10000)
> > + phydev->speed = SPEED_10000;
> > + else if (val & MV_AN_RESULT_SPD_1000)
> > + phydev->speed = SPEED_1000;
> > + else if (val & MV_AN_RESULT_SPD_100)
> > + phydev->speed = SPEED_100;
> > + else if (val & MV_AN_RESULT_SPD_10)
> > + phydev->speed = SPEED_10;
> > +
> > + phydev->duplex = DUPLEX_FULL;
> > + }
> > + }
> > +
> > + if (phydev->autoneg != AUTONEG_ENABLE) {
> > + val = genphy_c45_read_pma(phydev);
> > + if (val < 0)
> > + return val;
> > + }
> > +
> > + if ((phydev->interface == PHY_INTERFACE_MODE_SGMII ||
> > + phydev->interface == PHY_INTERFACE_MODE_10GKR) && phydev->link) {
> > + /* The PHY automatically switches its serdes interface (and
> > + * active PHYXS instance) between Cisco SGMII and 10GBase-KR
> > + * modes according to the speed. Florian suggests setting
> > + * phydev->interface to communicate this to the MAC. Only do
> > + * this if we are already in either SGMII or 10GBase-KR mode.
> > + */
>
> Hi Russell
>
> Just for my understanding. The MAC should check phydev->interface in
> its adjust_link function? Can we document this here please. I wounder
> if there is somewhere in the generic code we should also document
> this?
Yes, it would need to, since it would be unreliable to determine from
the link mode bits.
> > +static struct phy_driver mv3310_drivers[] = {
> > + {
> > + .phy_id = 0x002b09aa,
> > + .phy_id_mask = 0xffffffff,
>
> How about adding this ID to include/linux/marvell_phy.h? Or do you
> think this is not actually a Marvell ID, but some 3rd party which
> Marvell has licensed it from?
The vendor part is not the conventional Marvell ID. The 3310 appears
to be two or three different vendors IP all glued together, some of
which is in a way which violates the clause 45 register definitions.
Eg, there's multiple instances of PHYXS at 4K offsets, but C45
indicates that these are "reserved" and the place for vendor specific
stuff is in the two vendor MMDs. Same goes for some of the other
MMDs as well. I'll call these "MMD sub-blocks".
Some MMD sub-blocks have a Marvell ID in them, but others do not.
The OUI for the ID listed above is apparently "FiberHome Digital
Technology Co.,Ltd." but you'll also find the ID "0141 0daa" in some
of the MMD sub-blocks which corresponds with Marvell.
We can't use the sub-blocks though, because as I say, that's rather
non-standard and against the Clause 45 spec.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 2/5] net: phy: hook up clause 45 autonegotiation restart
From: Andrew Lunn @ 2017-06-01 13:05 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Florian Fainelli, netdev
In-Reply-To: <20170601125132.GY22219@n2100.armlinux.org.uk>
> > > +/**
> > > + * phy_restart_aneg - restart auto-negotiation
> > > + * @phydev: target phy_device struct
> > > + *
> > > + * Restart the autonegotiation on @phydev. Returns >= 0 on success or
> > > + * negative errno on error.
> > > + */
> > > +int phy_restart_aneg(struct phy_device *phydev)
> > > +{
> > > + int ret;
> > > +
> > > + if (phydev->is_c45)
> > > + ret = genphy_c45_restart_aneg(phydev);
> > > + else
> > > + ret = genphy_restart_aneg(phydev);
> > > +
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(phy_restart_aneg);
> >
> > Hi Russell
> >
> > Isn't the same sort of thing needed in phy_aneg_done()?
>
> No, because phy_aneg_done() already has hooks in it which PHY drivers
> can use to override the default C22 implementation.
>
> I did toy with providing a similar conditional in phy_aneg_done(), but
> decided it was better to use the existing hooks where present, rather
> than needlessly adding additional code.
Hi Russell
So you are saying a 10G PHY driver always needs to have a aneg_done
callback, even if it just needs to call phygen_c45_aneg_done?
This seems a bit error prone. I can see somebody writing a 10G driver,
leaving out aneg_done() and having the c22 version called. Is the read
of MII_BMSR likely to return 0xffff, since the register does not
exist? If so, genphy_aneg_done() is likely to always return
BMSR_ANEGCOMPLETE.
Seems like a trap waiting for somebody to fall into it. The additional
code might be worth it to avoid placing this trap.
Andrew
^ permalink raw reply
* [PATCH net] bnx2x: Fix Multi-Cos
From: Yuval Mintz @ 2017-06-01 12:57 UTC (permalink / raw)
To: davem, netdev; +Cc: eric.dumazet, Yuval Mintz
Apparently multi-cos isn't working for bnx2x quite some time -
driver implements ndo_select_queue() to allow queue-selection
for FCoE, but the regular L2 flow would cause it to modulo the
fallback's result by the number of queues.
The fallback would return a queue matching the needed tc
[via __skb_tx_hash()], but since the modulo is by the number of TSS
queues where number of TCs is not accounted, transmission would always
be done by a queue configured into using TC0.
Fixes: ada7c19e6d27 ("bnx2x: use XPS if possible for bnx2x_select_queue instead of pure hash")
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
Hi Dave,
Please consider applying this to `net'.
Thanks,
Yuval
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index eccb3d1..5f49334 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -1926,7 +1926,7 @@ u16 bnx2x_select_queue(struct net_device *dev, struct sk_buff *skb,
}
/* select a non-FCoE queue */
- return fallback(dev, skb) % BNX2X_NUM_ETH_QUEUES(bp);
+ return fallback(dev, skb) % (BNX2X_NUM_ETH_QUEUES(bp) * bp->max_cos);
}
void bnx2x_set_num_queues(struct bnx2x *bp)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 5/5] net: phy: add Marvell Alaska X 88X3310 10Gigabit PHY support
From: Andrew Lunn @ 2017-06-01 12:51 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, netdev
In-Reply-To: <E1dGNJc-000442-8H@rmk-PC.armlinux.org.uk>
> +static int mv3310_read_status(struct phy_device *phydev)
> +{
> + u32 mmd_mask = phydev->c45_ids.devices_in_package;
> + int val;
> +
> + /* The vendor devads do not report link status. Avoid the PHYXS
> + * instance as there are three, and its status depends on the MAC
> + * being appropriately configured for the negotiated speed.
> + */
> + mmd_mask &= ~(BIT(MDIO_MMD_VEND1) | BIT(MDIO_MMD_VEND2) |
> + BIT(MDIO_MMD_PHYXS));
> +
> + phydev->speed = SPEED_UNKNOWN;
> + phydev->duplex = DUPLEX_UNKNOWN;
> + phydev->lp_advertising = 0;
> + phydev->link = 0;
> + phydev->pause = 0;
> + phydev->asym_pause = 0;
> +
> + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_STAT1);
> + if (val < 0)
> + return val;
> +
> + if (val & MDIO_STAT1_LSTATUS)
> + return mv3310_read_10gbr_status(phydev);
> +
> + val = genphy_c45_read_link(phydev, mmd_mask);
> + if (val < 0)
> + return val;
> +
> + phydev->link = val > 0 ? 1 : 0;
> +
> + val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> + if (val < 0)
> + return val;
> +
> + if (val & MDIO_AN_STAT1_COMPLETE) {
> + val = genphy_c45_read_lpa(phydev);
> + if (val < 0)
> + return val;
> +
> + /* Read the link partner's 1G advertisment */
> + val = phy_read_mmd(phydev, MDIO_MMD_AN, MV_AN_STAT1000);
> + if (val < 0)
> + return val;
> +
> + phydev->lp_advertising |= mii_stat1000_to_ethtool_lpa_t(val);
> +
> + if (phydev->autoneg == AUTONEG_ENABLE) {
> + val = phy_read_mmd(phydev, MDIO_MMD_AN, MV_AN_RESULT);
> + if (val < 0)
> + return val;
> +
> + if (val & MV_AN_RESULT_SPD_10000)
> + phydev->speed = SPEED_10000;
> + else if (val & MV_AN_RESULT_SPD_1000)
> + phydev->speed = SPEED_1000;
> + else if (val & MV_AN_RESULT_SPD_100)
> + phydev->speed = SPEED_100;
> + else if (val & MV_AN_RESULT_SPD_10)
> + phydev->speed = SPEED_10;
> +
> + phydev->duplex = DUPLEX_FULL;
> + }
> + }
> +
> + if (phydev->autoneg != AUTONEG_ENABLE) {
> + val = genphy_c45_read_pma(phydev);
> + if (val < 0)
> + return val;
> + }
> +
> + if ((phydev->interface == PHY_INTERFACE_MODE_SGMII ||
> + phydev->interface == PHY_INTERFACE_MODE_10GKR) && phydev->link) {
> + /* The PHY automatically switches its serdes interface (and
> + * active PHYXS instance) between Cisco SGMII and 10GBase-KR
> + * modes according to the speed. Florian suggests setting
> + * phydev->interface to communicate this to the MAC. Only do
> + * this if we are already in either SGMII or 10GBase-KR mode.
> + */
Hi Russell
Just for my understanding. The MAC should check phydev->interface in
its adjust_link function? Can we document this here please. I wounder
if there is somewhere in the generic code we should also document
this?
> +static struct phy_driver mv3310_drivers[] = {
> + {
> + .phy_id = 0x002b09aa,
> + .phy_id_mask = 0xffffffff,
How about adding this ID to include/linux/marvell_phy.h? Or do you
think this is not actually a Marvell ID, but some 3rd party which
Marvell has licensed it from?
Andrew
^ permalink raw reply
* Re: [PATCH 2/5] net: phy: hook up clause 45 autonegotiation restart
From: Russell King - ARM Linux @ 2017-06-01 12:51 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, netdev
In-Reply-To: <20170601122350.GA9282@lunn.ch>
On Thu, Jun 01, 2017 at 02:23:50PM +0200, Andrew Lunn wrote:
> On Thu, Jun 01, 2017 at 11:26:36AM +0100, Russell King wrote:
> > genphy_restart_aneg() can only restart autonegotiation on clause 22
> > PHYs. Add a phy_restart_aneg() function which selects between the
> > clause 22 and clause 45 restart functionality depending on the PHY
> > type.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > drivers/net/phy/phy.c | 23 +++++++++++++++++++++--
> > include/linux/phy.h | 1 +
> > 2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index 82ab8fb82587..25b24789a409 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -149,6 +149,25 @@ static int phy_config_interrupt(struct phy_device *phydev, u32 interrupts)
> > return 0;
> > }
> >
> > +/**
> > + * phy_restart_aneg - restart auto-negotiation
> > + * @phydev: target phy_device struct
> > + *
> > + * Restart the autonegotiation on @phydev. Returns >= 0 on success or
> > + * negative errno on error.
> > + */
> > +int phy_restart_aneg(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + if (phydev->is_c45)
> > + ret = genphy_c45_restart_aneg(phydev);
> > + else
> > + ret = genphy_restart_aneg(phydev);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_restart_aneg);
>
> Hi Russell
>
> Isn't the same sort of thing needed in phy_aneg_done()?
No, because phy_aneg_done() already has hooks in it which PHY drivers
can use to override the default C22 implementation.
I did toy with providing a similar conditional in phy_aneg_done(), but
decided it was better to use the existing hooks where present, rather
than needlessly adding additional code.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Cherry pick commit *net: better skb->sender_cpu and skb->napi_id cohabitation* to 4.4 series?
From: Paul Menzel @ 2017-06-01 12:43 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet; +Cc: netdev, Alexei Starovoitov, it+linux-netdev
Dear David, dear Eric,
We noticed, that we manually apply commit 52bd2d62ce67 (net: better
skb->sender_cpu and skb->napi_id cohabitation), present since v4.5-rc1,
to our Linux 4.4 images.
Would it be useful to have it included in the 4.4 series?
Kind regards,
Paul
^ permalink raw reply
* [PATCH v3] net: don't call strlen on non-terminated string in dev_set_alias()
From: Alexander Potapenko @ 2017-06-01 12:38 UTC (permalink / raw)
To: dvyukov, kcc, edumazet, davem, stephen; +Cc: linux-kernel, netdev
KMSAN reported a use of uninitialized memory in dev_set_alias(),
which was caused by calling strlcpy() (which in turn called strlen())
on the user-supplied non-terminated string.
Signed-off-by: Alexander Potapenko <glider@google.com>
---
v3: removed the multi-line comment
v2: fixed an off-by-one error spotted by Dmitry Vyukov
For the record, here is the KMSAN report:
==================================================================
BUG: KMSAN: use of unitialized memory in strlcpy+0x8b/0x1b0
CPU: 0 PID: 1062 Comm: probe Not tainted 4.11.0-rc5+ #2763
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:16
dump_stack+0x143/0x1b0 lib/dump_stack.c:52
kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:1016
__kmsan_warning_32+0x66/0xb0 mm/kmsan/kmsan_instr.c:491
strlen lib/string.c:484
strlcpy+0x8b/0x1b0 lib/string.c:144
dev_set_alias+0x295/0x360 net/core/dev.c:1255
do_setlink+0xfe5/0x5750 net/core/rtnetlink.c:2007
rtnl_setlink+0x469/0x4f0 net/core/rtnetlink.c:2249
rtnetlink_rcv_msg+0x5da/0xb40 net/core/rtnetlink.c:4107
netlink_rcv_skb+0x339/0x5a0 net/netlink/af_netlink.c:2339
rtnetlink_rcv+0x83/0xa0 net/core/rtnetlink.c:4113
netlink_unicast_kernel net/netlink/af_netlink.c:1272
netlink_unicast+0x13b7/0x1480 net/netlink/af_netlink.c:1298
netlink_sendmsg+0x10b8/0x10f0 net/netlink/af_netlink.c:1844
sock_sendmsg_nosec net/socket.c:633
sock_sendmsg net/socket.c:643
sock_write_iter+0x395/0x440 net/socket.c:857
call_write_iter ./include/linux/fs.h:1733
new_sync_write fs/read_write.c:497
__vfs_write+0x619/0x700 fs/read_write.c:510
vfs_write+0x47e/0x8a0 fs/read_write.c:558
SYSC_write+0x17e/0x2f0 fs/read_write.c:605
SyS_write+0x87/0xb0 fs/read_write.c:597
entry_SYSCALL_64_fastpath+0x13/0x94 arch/x86/entry/entry_64.S:204
RIP: 0033:0x4052f1
RSP: 002b:00007fde1ed05d80 EFLAGS: 00000293 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00000000004052f1
RDX: 0000000000000026 RSI: 00000000006cf0c0 RDI: 0000000000000032
RBP: 00007fde1ed05da0 R08: 00007fde1ed06700 R09: 00007fde1ed06700
R10: 00007fde1ed069d0 R11: 0000000000000293 R12: 0000000000000000
R13: 0000000000000000 R14: 00007fde1ed069c0 R15: 00007fde1ed06700
origin: 00000000aec00057
save_stack_trace+0x59/0x60 arch/x86/kernel/stacktrace.c:59
kmsan_save_stack_with_flags mm/kmsan/kmsan.c:352
kmsan_internal_poison_shadow+0xb1/0x1a0 mm/kmsan/kmsan.c:247
kmsan_poison_shadow+0x6d/0xc0 mm/kmsan/kmsan.c:260
slab_alloc_node mm/slub.c:2743
__kmalloc_node_track_caller+0x1f4/0x390 mm/slub.c:4349
__kmalloc_reserve net/core/skbuff.c:138
__alloc_skb+0x2cd/0x740 net/core/skbuff.c:231
alloc_skb ./include/linux/skbuff.h:933
netlink_alloc_large_skb net/netlink/af_netlink.c:1144
netlink_sendmsg+0x934/0x10f0 net/netlink/af_netlink.c:1819
sock_sendmsg_nosec net/socket.c:633
sock_sendmsg net/socket.c:643
sock_write_iter+0x395/0x440 net/socket.c:857
call_write_iter ./include/linux/fs.h:1733
new_sync_write fs/read_write.c:497
__vfs_write+0x619/0x700 fs/read_write.c:510
vfs_write+0x47e/0x8a0 fs/read_write.c:558
SYSC_write+0x17e/0x2f0 fs/read_write.c:605
SyS_write+0x87/0xb0 fs/read_write.c:597
entry_SYSCALL_64_fastpath+0x13/0x94 arch/x86/entry/entry_64.S:204
==================================================================
and the reproducer:
==================================================================
#include <pthread.h>
int sock = -1;
char buf[] = "\x26\x00\x00\x00\x13\x00\x47\xf1\x07\x01\xc1\xb0"
"\x0e\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00"
"\x09\xef\x18\xff\xff\x00\xf1\x32\x05\x00\x14\x00"
"\x6e\x35";
void do_work(int arg) {
switch (arg) {
case 0:
sock = socket(0x10, 0x3, 0x0);
break;
case 1:
write(sock, buf, 0x26);
break;
}
}
void *thread_fn(void *arg) {
int actual_arg = (int)arg;
int iter = 10000, i;
for (i = 0; i < iter; i++) {
do_work(actual_arg);
usleep(100);
}
return NULL;
}
int main() {
pthread_t thr[4];
int i;
for (i = 0; i < 4; i++) {
pthread_create(&thr, NULL, thread_fn, (void*)(i % 2));
}
sleep(10);
return 0;
}
==================================================================
---
net/core/dev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index fca407b4a6ea..3e3b29133cc9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1254,7 +1254,9 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
return -ENOMEM;
dev->ifalias = new_ifalias;
- strlcpy(dev->ifalias, alias, len+1);
+ /* alias comes from the userspace and may not be zero-terminated. */
+ memcpy(dev->ifalias, alias, len);
+ dev->ifalias[len] = 0;
return len;
}
--
2.13.0.219.gdb65acc882-goog
^ 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