* 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: 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: 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: [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: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument
From: Djalal Harouni @ 2017-06-01 14:56 UTC (permalink / raw)
To: Kees Cook
Cc: Serge E. Hallyn, Rusty Russell, David S . Miller, Jessica Yu,
LKML, Network Development, linux-security-module,
kernel-hardening@lists.openwall.com, Andy Lutomirski,
Andrew Morton, James Morris, Paul Moore, Stephen Smalley,
Greg Kroah-Hartman, Tetsuo Handa, Ingo Molnar, Linux API,
Dongsu Park, Casey Schaufler, Jonathan Corbet,
Arnaldo Carvalho de Melo
In-Reply-To: <CAGXu5jK5exWBV+ub8ys86Ubu1vKwN=miOZxwp3zdWaWEfuJddQ@mail.gmail.com>
On Tue, May 30, 2017 at 7:59 PM, Kees Cook <keescook@google.com> wrote:
[...]
>>> I see a few options:
>>>
>>> 1) keep what you have for v4, and hope other places don't use
>>> __request_module. (I'm not a fan of this.)
>>
>> Yes even if it is documented I wouldn't bet on it, though. :-)
>
> Okay, we seem to agree: we'll not use #1.
>
>>> 2) switch the logic on autoload==1 from OR to AND: both the specified
>>> caps _and_ CAP_SYS_MODULE are required. (This seems like it might make
>>> autoload==1 less useful.)
>>
>> That will restrict some userspace that works only with CAP_NET_ADMIN.
>
> Nor #2.
>
>>> 3) use the request_module_cap() outlined above, which requires that
>>> modules being loaded under a CAP_SYS_MODULE-aliased capability are at
>>> least restricted to a subset of kernel module names.
>>
>> This one tends to allow usability.
>
> Right, discussed below...
>
>>> 4) same as 3 but also insert autoload==2 level that switches from OR
>>> to AND (bumping existing ==2 to ==3).
>>
>> I wouldn't expose autoload to callers, I think it is better if it
>> stays a property of the module subsystem. But lets use the bump idea,
>> please see below.
>
> If we can't agree below, I think #4 would be a good way to allow for
> both states.
Ok!
>>> What do you think?
>>
>> Ok so given that we already have modules_autoload_mode=2 disabled,
>> maybe we go with 3) like this ?
>>
>> int __request_module(bool wait, int required_cap, const char *prefix,
>> const char *name, ...);
>> #define request_module(mod...) \
>> __request_module(true, -1, NULL, mod)
>> #define request_module_cap(required_cap, prefix, mod...) \
>> __request_module(true, required_cap, prefix, mod)
>>
>> and we require allow_cap and prefix to be set.
>>
>> request_module_cap(CAP_NET_ADMIN, "netdev-", "%s", name) for
>> net/core/dev_ioctl.c:dev_load()
>>
>> request_module_cap(CAP_NET_ADMIN, "tcp_", "%s", name) for
>> net/ipv4/tcp_cong.c functions.
>>
>>
>> Then
>> __request_module()
>> -> security_kernel_module_request(module_name, required_cap, prefix)
>> -> may_autoload_module(current, module_name, required_cap, prefix)
>>
>>
>> And update may_autoload_module() as below ? we hard code CAP_NET_ADMIN
>> and CAP_SYS_MODULE inside and make them the only capabilities needed
>> for a privileged auto-load operation.
>
> I still think making a specific exception for CAP_NET_ADMIN is not the
> right solution, instead allowing for non-CAP_SYS_MODULE caps when
> using a distinct prefix.
Alright! I would have loved to avoid capabilities game, but these
patches also use them... so worst scenario is the per-task can always
be set, "task->module_autoload_mode=2" and block it if necessary.
>> request_module_cap(CAP_SYS_MODULE, ...) or
>> request_module_cap(CAP_NET_ADMIN, ...) if the autoload should be a
>> privileged operation.
>>
>> Kees will this work ?
>>
>> Jessica, Rusty, Serge. What do you think ? I definitively think that
>> module_autoload should be contained only inside the module subsystem..
>
> I'd change it like this:
>
>> +int may_autoload_module(struct task_struct *task, char *kmod_name,
>> + int require_cap, char *prefix)
>> +{
>> + unsigned int autoload;
>> + int module_require_cap = 0;
>
> I'd initialize this to module_require_cap = CAP_SYS_MODULE;
Ok, please see below.
>> +
>> + if (require_cap > 0) {
>> + if (prefix == NULL || *prefix == '\0')
>> + return -EPERM;
>
> Since an unprefixed module load should only be CAP_SYS_MODULE, change
> the above "if" to:
>
> if (require_cap > 0 && prefix != NULL && *prefix != '\0')
>
>> +
>> + /*
>> + * We only allow CAP_SYS_MODULE or CAP_NET_ADMIN for
>> + * 'netdev-%s' modules for backward compatibility.
>> + * Please do not overload capabilities.
>> + */
>> + if (require_cap == CAP_SYS_MODULE ||
>> + require_cap == CAP_NET_ADMIN)
>> + module_require_cap = require_cap;
>> + else
>> + return -EPERM;
>> + }
>
> And then drop all these checks, leaving only:
>
> module_require_cap = require_cap;
>
>> +
>> + /* Get max value of sysctl and task "modules_autoload_mode" */
>> + autoload = max_t(unsigned int, modules_autoload_mode,
>> + task->modules_autoload_mode);
>> +
>> + /*
>> + * If autoload is disabled then fail here and not bother at all
>> + */
>> + if (autoload == MODULES_AUTOLOAD_DISABLED)
>> + return -EPERM;
>> +
>> + /*
>> + * If caller require capabilities then we may not allow
>> + * automatic module loading. We should not bypass callers.
>> + * This allows to support networking code that uses CAP_NET_ADMIN
>> + * for some aliased 'netdev-%s' modules.
>> + *
>> + * Explicitly bump autoload here if necessary
>> + */
>> + if (module_require_cap && autoload == MODULES_AUTOLOAD_ALLOWED)
>> + autoload = MODULES_AUTOLOAD_PRIVILEGED;
>
> I don't see a reason to bump the autoload level.
>
>> +
>> + if (autoload == MODULES_AUTOLOAD_ALLOWED)
>> + return 0;
>
> This test can be moved to above the AUTOLOAD_DISABLED test.
>
>> + else if(autoload == MODULES_AUTOLOAD_PRIVILEGED) {
>> + /*
>> + * If module auto-load is a privileged operation then check
>> + * if capabilities are set.
>> + */
>> + if (capable(CAP_SYS_MODULE) ||
>> + (module_require_cap && capable(module_require_cap)))
>> + return 0;
>> + }
>
> This test could drop the explicit CAP_SYS_MODULE test and just rely on
> module_require_cap.
>
>> +
>> + return -EPERM;
>> +}
>> +
>
> So, I would suggest:
Ok Kees, I will update based on your feedback, except for the
following, please see below and let me know :-)
>
> int may_autoload_module(struct task_struct *task, char *kmod_name,
> int require_cap, char *prefix)
> {
> unsigned int autoload;
> int module_require_cap;
>
> if (autoload == MODULES_AUTOLOAD_DISABLED)
> return -EPERM;
>
> /* Get max value of sysctl and task "modules_autoload_mode" */
> autoload = max_t(unsigned int, modules_autoload_mode,
> task->modules_autoload_mode);
>
> if (autoload == MODULES_AUTOLOAD_ALLOWED)
> return 0;
I don't think that the MODULES_AUTOLOAD_ALLOWED check here at this
place is the best thing to do.
If we remove the capable(CAP_NET_ADMIN) from net/core/dev_ioctl:dev_load()
http://elixir.free-electrons.com/linux/v4.12-rc3/source/net/core/dev_ioctl.c#L369
Or if future changes (accidental) remove that capable(CAP_NET_ADMIN)
and replace it with:
request_module_cap(CAP_NET_ADMIN, "netdev-", "%s", name);
Then we will check the requested capability *after* autoload allowed
as it is in this example, it should be checked *before* the autoload
allowed:
// Check required capability before this
if (autoload == MODULES_AUTOLOAD_ALLOWED)
return 0;
This way we are still safe we do not downgrade the required capability
that was requested by the calling subsystem based on
MODULES_AUTOLOAD_ALLOWED. If networking code or any other code thinks
that we need CAP_X to load a module then we should honor it. So we do
not break current usage by introducing the "modules_autoload_mode", it
should be set regardless of the autoload mode. Especially since
modules autoload mode is 0 by default. This avoids breaking current
rule to require CAP_NET_ADMIN for 'netdevè-%'
> /*
> * It should be impossible for autoload to have any other
> * value at this point, so explicitly reject all other states.
> */
> if (autoload != MODULES_AUTOLOAD_PRIVILEGED)
> return -EPERM;
>
> /* Verify that alternate capabilities requirements had a prefix. */
> if (require_cap > 0 && prefix != NULL && *prefix != '\0')
> module_require_cap = require_cap;
> else
> module_require_cap = CAP_SYS_MODULE;
>
> return capable(module_require_cap);
So with your code, but I really think that we should treat
MODULES_AUTOLOAD_ALLOWED with special care in regard of the passed
capabilities, so:
module_require_cap = 0;
if (autoload == MODULES_AUTOLOAD_DISABLED)
return -EPERM;
if (autoload == MODULES_AUTOLOAD_PRIVILEGED || require_cap > 0) {
if (prefix != NULL && *prefix != '\0')
/*
* Allow non-CAP_SYS_MODULE caps when
* using a distinct prefix.
*/
module_require_cap = require_cap;
else
/*
* Otherwise always require CAP_SYS_MODULE if no
* valid prefix. Callers that do not provide a
valid prefix
* should not provide a require_cap > 0
*/
module_require_cap = CAP_SYS_MODULE;
}
/* If autoload allowed and 'module_require_cap' was *never*
set, allow */
if (module_require_cap == 0 && autoload == MODULES_AUTOLOAD_ALLOWED)
return 0;
return capable(module_require_cap) ? 0 : -EPERM;
> }
>
Maybe you will agree :-) ?
BTW Kees, also in next version I won't remove the
capable(CAP_NET_ADMIN) check from [1]
even if there is the new request_module_cap(), I would like it to be
in a different patches, this way we go incremental
and maybe it is better to merge what we have now ? and follow up
later, and of course if other maintainers agree too!
I just need a bit of free time to check again everything and will send
a v5 with all requested changes.
Thank you Kees for your time!
[1] http://elixir.free-electrons.com/linux/v4.12-rc3/source/net/core/dev_ioctl.c#L369
^ permalink raw reply
* [PATCH V7 net-next iproute] ip: Add IFLA_EVENT output to ip monitor
From: Vladislav Yasevich @ 2017-06-01 15:01 UTC (permalink / raw)
To: netdev; +Cc: stephen, dsahern, Vladislav Yasevich
Add IFLA_EVENT output so that event types can be viewed with
'monitor' command. This gives a little more information for why
a given message was received.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
v7: change the names to make it more generic then just netdev events.
Updated initialization.
include/linux/if_link.h | 11 +++++++++++
ip/ipaddress.c | 22 ++++++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 5a3a048..c0a6769 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -157,6 +157,7 @@ enum {
IFLA_GSO_MAX_SIZE,
IFLA_PAD,
IFLA_XDP,
+ IFLA_EVENT,
__IFLA_MAX
};
@@ -909,4 +910,14 @@ enum {
#define IFLA_XDP_MAX (__IFLA_XDP_MAX - 1)
+enum {
+ IFLA_EVENT_NONE,
+ IFLA_EVENT_REBOOT, /* internal reset / reboot */
+ IFLA_EVENT_FEATURES, /* change in offload features */
+ IFLA_EVENT_BONDING_FAILOVER, /* hange in active slave */
+ IFLA_EVENT_NOTIFY_PEERS, /* re-sent grat. arp/ndisc */
+ IFLA_EVENT_IGMP_RESEND, /* re-sent IGMP JOIN */
+ IFLA_EVENT_BONDING_OPTIONS, /* change in bonding options */
+};
+
#endif /* _LINUX_IF_LINK_H */
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index b8d9c7d..cea4389 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -753,6 +753,25 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
return 0;
}
+static const char *link_events[] = {
+ [IFLA_EVENT_NONE] = "NONE",
+ [IFLA_EVENT_REBOOT] = "REBOOT",
+ [IFLA_EVENT_FEATURES] = "FEATURE CHANGE",
+ [IFLA_EVENT_BONDING_FAILOVER] = "BONDING FAILOVER",
+ [IFLA_EVENT_NOTIFY_PEERS] = "NOTIFY PEERS",
+ [IFLA_EVENT_IGMP_RESEND] = "RESEND IGMP",
+ [IFLA_EVENT_BONDING_OPTIONS] = "BONDING OPTION"};
+
+static void print_link_event(FILE *f, __u32 event)
+{
+ if (event >= ARRAY_SIZE(link_events))
+ fprintf(f, "event %d ", event);
+ else {
+ if (event)
+ fprintf(f, "event %s ", link_events[event]);
+ }
+}
+
int print_linkinfo(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
{
@@ -858,6 +877,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
if (filter.showqueue)
print_queuelen(fp, tb);
+ if (tb[IFLA_EVENT])
+ print_link_event(fp, rta_getattr_u32(tb[IFLA_EVENT]));
+
if (!filter.family || filter.family == AF_PACKET || show_details) {
SPRINT_BUF(b1);
fprintf(fp, "%s", _SL_);
--
2.7.4
^ permalink raw reply related
* [PATCH net] net: bridge: start hello timer only if device is up
From: Nikolay Aleksandrov @ 2017-06-01 15:07 UTC (permalink / raw)
To: netdev
Cc: davem, roopa, Nikolay Aleksandrov, Xin Long, Ivan Vecera,
Sebastian Ott
In-Reply-To: <06ef2ff1-f29d-a163-3226-7bd43c7a407c@cumulusnetworks.com>
When the transition of NO_STP -> KERNEL_STP was fixed by always calling
mod_timer in br_stp_start, it introduced a new regression which causes
the timer to be armed even when the bridge is down, and since we stop
the timers in its ndo_stop() function, they never get disabled if the
device is destroyed before it's upped.
To reproduce:
$ while :; do ip l add br0 type bridge hello_time 100; brctl stp br0 on;
ip l del br0; done;
CC: Xin Long <lucien.xin@gmail.com>
CC: Ivan Vecera <cera@cera.cz>
CC: Sebastian Ott <sebott@linux.vnet.ibm.com>
Reported-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Fixes: 6d18c732b95c ("bridge: start hello_timer when enabling KERNEL_STP in br_stp_start")
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
Sebastian it'd be great if you can test the patch as well.
net/bridge/br_stp_if.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
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);
}
--
2.1.4
^ 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 15:10 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: <CACSApvbKwbX2u16V1oQfmHC7SvGZgk-KehA9tashjQAtj0bB0Q@mail.gmail.com>
Hi!
> Thank you for the confirmation. Could you please try the following
> patch to see if it fixes your issue?
Does not seem to help, I still got the same bussy loop.
--
Cyril Hrubis
chrubis@suse.cz
^ 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 15:15 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: <20170601151003.GB10526@rei.suse.de>
On Thu, Jun 1, 2017 at 11:10 AM, Cyril Hrubis <chrubis@suse.cz> wrote:
>> Thank you for the confirmation. Could you please try the following
>> patch to see if it fixes your issue?
>
> Does not seem to help, I still got the same bussy loop.
Thank you for trying the patch. Unfortunately, I can't reproduce on my
machines here.Would you humor me with another one? Thank you!
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5a726161f4e4..49207298fcea 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3742,7 +3742,8 @@ EXPORT_SYMBOL(sock_queue_err_skb);
static bool is_icmp_err_skb(const struct sk_buff *skb)
{
return skb && (SKB_EXT_ERR(skb)->ee.ee_origin == SO_EE_ORIGIN_ICMP ||
- SKB_EXT_ERR(skb)->ee.ee_origin == SO_EE_ORIGIN_ICMP6);
+ SKB_EXT_ERR(skb)->ee.ee_origin == SO_EE_ORIGIN_ICMP6 ||
+ SKB_EXT_ERR(skb)->ee.ee_origin == SO_EE_ORIGIN_LOCAL);
}
struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
^ permalink raw reply related
* Re: [PATCH v2 net-next 1/3] perf, bpf: Add BPF support to all perf_event types
From: Alexei Starovoitov @ 2017-06-01 15:21 UTC (permalink / raw)
To: Peter Zijlstra
Cc: David S . Miller, Brendan Gregg, Daniel Borkmann, Teng Qin,
netdev, linux-kernel
In-Reply-To: <20170601133216.oxroqy4z5qjwvrdm@hirez.programming.kicks-ass.net>
On 6/1/17 6:32 AM, Peter Zijlstra wrote:
> 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).
Thanks! Will test it and report back.
^ permalink raw reply
* Re: [PATCH net-next] ipv4: route: restore skb_dst_set in inet_rtm_getroute
From: David Miller @ 2017-06-01 15:34 UTC (permalink / raw)
To: roopa; +Cc: netdev, john.fastabend, dsahern
In-Reply-To: <1496296405-61386-1-git-send-email-roopa@cumulusnetworks.com>
From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Wed, 31 May 2017 22:53:25 -0700
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> recent updates to inet_rtm_getroute dropped skb_dst_set in
> inet_rtm_getroute. This patch restores it because it is
> needed to release the dst correctly.
>
> Fixes: 3765d35ed8b9 ("net: ipv4: Convert inet_rtm_getroute to rcu versions of route lookup")
> Reported-by: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
Applied.
^ permalink raw reply
* Re: commit f5f99309 (sock: do not set sk_err in sock_dequeue_err_skb) has broken ping
From: Cyril Hrubis @ 2017-06-01 15:36 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: <CACSApvanT0ZRFH0ZBDLqygkH6Ue_CCVQBzVL9fCmZE_BndYEog@mail.gmail.com>
Hi!
> >> Thank you for the confirmation. Could you please try the following
> >> patch to see if it fixes your issue?
> >
> > Does not seem to help, I still got the same bussy loop.
>
> Thank you for trying the patch. Unfortunately, I can't reproduce on my
> machines here.Would you humor me with another one? Thank you!
Still does not help.
I've modified the ping binary (on the top of you patch that reads the
error queue) to dump the error structure.
It seems to repeatedly produce (until I plug the cable back):
ee_errno = 113 ee_origin = 2 ee_type = 3 ee_code = 1 ee_info = 0 ee_data = 0
So we get EHOSTUNREACH on SO_EE_ORIGIN_ICMP.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply
* Re: [for-next 4/6] net/mlx5: FPGA, Add basic support for Innova
From: Jason Gunthorpe @ 2017-06-01 15:37 UTC (permalink / raw)
To: Ilan Tayari
Cc: Alexei Starovoitov, Saeed Mahameed, David S. Miller, Doug Ledford,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
jsorensen-b10kYP2dOMg@public.gmane.org, Andy Shevchenko,
linux-fpga-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Alan Tull,
yi1.li-VuQAYsv1563Yd54FQh9/CA@public.gmane.org, Boris Pismenny
In-Reply-To: <AM4PR0501MB19409139227E11A4A7F82F0FDBF30-dp/nxUn679gfNUYDR5dMTsDSnupUy6xnnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
On Mon, May 29, 2017 at 04:09:06PM +0000, Ilan Tayari wrote:
> > For IPSec, this is already in the kernel.
> > See this patchset:
> > http://www.mail-archive.com/netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg162876.html
>
> Sorry, I pointed at the RFC by mistake.
>
> This is the relevant pull request:
> https://patchwork.ozlabs.org/patch/752707/
This is connecting ipsec to a netdev, while Innova seems to be a
network connected ipsec accelerator configured using IP packets.
Those two things don't seem to be the same.
Jason
--
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
* Re: [PATCH] qlcnic: Fix a sleep-in-atomic bug in qlcnic_82xx_hw_write_wx_2M and qlcnic_82xx_hw_read_wx_2M
From: David Miller @ 2017-06-01 15:42 UTC (permalink / raw)
To: baijiaju1990
Cc: harish.patil, manish.chopra, Dept-GELinuxNICDev, netdev,
linux-kernel
In-Reply-To: <1496305090-17814-1-git-send-email-baijiaju1990@163.com>
From: Jia-Ju Bai <baijiaju1990@163.com>
Date: Thu, 1 Jun 2017 16:18:10 +0800
> The driver may sleep under a write spin lock, and the function
> call path is:
> qlcnic_82xx_hw_write_wx_2M (acquire the lock by write_lock_irqsave)
> crb_win_lock
> qlcnic_pcie_sem_lock
> usleep_range
> qlcnic_82xx_hw_read_wx_2M (acquire the lock by write_lock_irqsave)
> crb_win_lock
> qlcnic_pcie_sem_lock
> usleep_range
>
> To fix it, the usleep_range is replaced with udelay.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
Applied.
^ permalink raw reply
* Re: [PATCH 2/5] net: phy: hook up clause 45 autonegotiation restart
From: Russell King - ARM Linux @ 2017-06-01 15:47 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, netdev
In-Reply-To: <20170601131955.GH9282@lunn.ch>
On Thu, Jun 01, 2017 at 03:19:55PM +0200, Andrew Lunn wrote:
> 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.
Yes, C45 allows the PHYs to answer C22 as well, but then they have to
implement the C22 register set. Such a PHY would be out of spec,
especially as what you're suggesting is that it answers C22 cycles
and fails to implement MII_BMSR. I also think that there's a comment
in the 802.3 specs that says that unimplemented registers are to
return zero, not 0xffff.
So, if there is a non-compliant PHY like that, I think the time to
address it is when such a broken situation appears, rather than
engineering additional complexity before we know we need it.
--
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 net] netlink: don't send unknown nsid
From: David Miller @ 2017-06-01 15:50 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev, fbl
In-Reply-To: <1496304007-1962-1-git-send-email-nicolas.dichtel@6wind.com>
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu, 1 Jun 2017 10:00:07 +0200
> The NETLINK_F_LISTEN_ALL_NSID otion enables to listen all netns that have a
> nsid assigned into the netns where the netlink socket is opened.
> The nsid is sent as metadata to userland, but the existence of this nsid is
> checked only for netns that are different from the socket netns. Thus, if
> no nsid is assigned to the socket netns, NETNSA_NSID_NOT_ASSIGNED is
> reported to the userland. This value is confusing and useless.
> After this patch, only valid nsid are sent to userland.
>
> Reported-by: Flavio Leitner <fbl@sysclose.org>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Applied, thank you.
^ permalink raw reply
* Re: [net-next] team: add macro MODULE_ALIAS_TEAM_MODE for team mode alias
From: David Miller @ 2017-06-01 15:50 UTC (permalink / raw)
To: zhangshengju; +Cc: netdev, jiri
In-Reply-To: <1496302622-9958-1-git-send-email-zhangshengju@cmss.chinamobile.com>
From: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
Date: Thu, 1 Jun 2017 15:37:02 +0800
> Add a new macro MODULE_ALIAS_TEAM_MODE to unify and simplify the
> declaration of team mode alias.
>
> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
Jiri, please review.
^ permalink raw reply
* Re: [PATCH net] ibmvnic: Update module author
From: Thomas Falcon @ 2017-06-01 15:53 UTC (permalink / raw)
To: David Miller; +Cc: netdev, nfont, jallen
In-Reply-To: <20170531.171203.1567468797463479514.davem@davemloft.net>
On 05/31/2017 04:12 PM, David Miller wrote:
> From: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
> Date: Wed, 31 May 2017 15:35:31 -0500
>
>> Update the module author to be one of the original co-maintainers
>> of the driver. The original author left the project long ago and
>> so far has not responded to emails sent to the listed address.
>>
>> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
>> Signed-off-by: Thomas Falcon <tlfalcon@linux.vnet.ibm.com>
> It's MODULE_AUTHOR not MODULE_MAINTAINER.
>
Would it be better to include a second or more authors and/or remove the original author's mailing address, which isn't valid anymore? How should this be handled?
^ permalink raw reply
* Re: [net] vxlan: fix use-after-free on deletion
From: David Miller @ 2017-06-01 15:54 UTC (permalink / raw)
To: markb; +Cc: jbenc, roopa, netdev
In-Reply-To: <1496306615-770-1-git-send-email-markb@mellanox.com>
From: Mark Bloch <markb@mellanox.com>
Date: Thu, 1 Jun 2017 11:43:35 +0300
> Adding a vxlan interface to a socket isn't symmetrical, while adding
> is done in vxlan_open() the deletion is done in vxlan_dellink().
> This can cause a use-after-free error when we close the vxlan
> interface before deleting it.
>
> We add vxlan_vs_del_dev() to match vxlan_vs_add_dev() and call
> it from vxlan_stop() to match the call from vxlan_open().
>
> Signed-off-by: Mark Bloch <markb@mellanox.com>
Please add an appropriate Fixes: tag.
Thank you.
^ permalink raw reply
* Re: [PATCH net V3] net/mlx4: Fix the check in attaching steering rules
From: David Miller @ 2017-06-01 15:56 UTC (permalink / raw)
To: tariqt; +Cc: netdev, eranbe, ogerlitz, talatb
In-Reply-To: <1496308606-13653-1-git-send-email-tariqt@mellanox.com>
From: Tariq Toukan <tariqt@mellanox.com>
Date: Thu, 1 Jun 2017 12:16:46 +0300
> From: Talat Batheesh <talatb@mellanox.com>
>
> Our previous patch (cited below) introduced a regression
> for RAW Eth QPs.
>
> Fix it by checking if the QP number provided by user-space
> exists, hence allowing steering rules to be added for valid
> QPs only.
>
> Fixes: 89c557687a32 ("net/mlx4_en: Avoid adding steering rules with ...")
Please do not truncate the commit header line text, always provide the
whole thing and do not break up the Fixes: line with newlines even if
the line exceeds 80 columns so people can grep it into the logs.
Thank you.
^ permalink raw reply
* Re: [PATCH net-next 2/3] udp: avoid a cache miss on dequeue
From: Eric Dumazet @ 2017-06-01 15:58 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, David S. Miller, Eric Dumazet
In-Reply-To: <1496313592.4872.14.camel@redhat.com>
On Thu, 2017-06-01 at 12:39 +0200, Paolo Abeni wrote:
> On Wed, 2017-05-31 at 10:00 -0700, Eric Dumazet wrote:
> > On Mon, 2017-05-29 at 17:27 +0200, Paolo Abeni wrote:
> > > Since UDP no more uses sk->destructor, we can clear completely
> > > the skb head state before enqueuing.
> >
> > ...
> >
> > > @@ -1739,6 +1740,9 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> > > sk_mark_napi_id_once(sk, skb);
> > > }
> > >
> > > + /* drop all pending head states; dst, nf and sk are dropped by caller */
> > > + secpath_reset(skb);
> > > +
> >
> > I wonder if using skb_release_head_state() would be more appropriate ?
> >
> > Surely more descriptive and probably not more expensive since all
> > cache lines should be already hot at this point.
>
> Thank you for reviewing this.
>
> I would prefer not adding more code to the core, but I think we would
> need something new, like:
>
> skb_reset_head_state()
> {
> skb_dst_drop(skb);
> secpath_reset(skb);
> nf_reset(skb);
> skb_orphan(skb);
> }
>
> because elsewhere the skb could be in inconsistent state: skb->sp !=
> NULL but with its refcount is already decremented. WDYT?
I do not believe skb->sk is set anymore in UDP receive path.
If early demux sets skb->sk for a moment, skb_steal_sock() would set
skb->sk back to NULL
^ permalink raw reply
* Re: [PATCH net-next] ppp: remove unnecessary bh disable in xmit path
From: David Miller @ 2017-06-01 16:01 UTC (permalink / raw)
To: gfree.wind; +Cc: paulus, linux-ppp, g.nault, netdev
In-Reply-To: <1496311119-59204-1-git-send-email-gfree.wind@vip.163.com>
From: gfree.wind@vip.163.com
Date: Thu, 1 Jun 2017 17:58:39 +0800
> From: Gao Feng <gfree.wind@vip.163.com>
>
> Since the commit 55454a565836 ("ppp: avoid dealock on recursive xmit"),
> the PPP xmit path is protected by wrapper functions which disable the
> bh already. So it is unnecessary to disable the bh again in the real
> xmit path.
>
> Signed-off-by: Gao Feng <gfree.wind@vip.163.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH 0/5] Add phylib support for MV88X3310 10G phy
From: David Miller @ 2017-06-01 16:07 UTC (permalink / raw)
To: linux; +Cc: andrew, f.fainelli, devicetree, mark.rutland, netdev, robh+dt
In-Reply-To: <20170601102327.GF27796@n2100.armlinux.org.uk>
From: Russell King - ARM Linux <linux@armlinux.org.uk>
Date: Thu, 1 Jun 2017 11:23:27 +0100
> This patch series adds support for the Marvell 88x3310 PHY found on
> the SolidRun Macchiatobin board.
Andrew has asked for some comment documentation additions to patch #5
so I'm expecting at least one respin of this series ;-)
^ permalink raw reply
* Re: [PATCH net-next 1/4] net/flow_dissector: add support for dissection of misc ip header fields
From: Or Gerlitz @ 2017-06-01 16:08 UTC (permalink / raw)
To: David Miller
Cc: Tom Herbert, Jiri Pirko, Or Gerlitz, Linux Netdev List,
Saeed Mahameed, Roi Dayan, Paul Blakey
In-Reply-To: <20170530.121800.676375959648055594.davem@davemloft.net>
On Tue, May 30, 2017 at 7:18 PM, David Miller <davem@davemloft.net> wrote:
> The top level merge commit explains everything, and since the patches
> are grouped together logically, even if you just look at the merge
> commit in the GIT tree, it is mentioned.
Hi Dave,
Do you want to see a repost here or you are going to process the
series I submitted from patchworks?
Or.
^ permalink raw reply
* Re: [PATCH net] cxgb4: update latest firmware version supported
From: David Miller @ 2017-06-01 16:10 UTC (permalink / raw)
To: ganeshgr; +Cc: netdev, nirranjan
In-Reply-To: <1496317290-26368-1-git-send-email-ganeshgr@chelsio.com>
From: Ganesh Goudar <ganeshgr@chelsio.com>
Date: Thu, 1 Jun 2017 17:11:30 +0530
> Change t4fw_version.h to update latest firmware version
> number to 1.16.45.0.
>
> Signed-off-by: Ganesh Goudar <ganeshgr@chelsio.com>
Applied.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox