* Re: [PATCH] dp83640: Ensure against premature access to PHY registers after reset
From: Esben Haabendal @ 2018-04-06 17:06 UTC (permalink / raw)
To: David Miller; +Cc: andrew, netdev, richardcochran, f.fainelli, linux-kernel
In-Reply-To: <20180406.111337.1908168293065420432.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Fri, 6 Apr 2018 16:14:10 +0200
>
>> On Fri, Apr 06, 2018 at 04:05:40PM +0200, Esben Haabendal wrote:
>>> From: Esben Haabendal <eha@deif.com>
>>>
>>> Signed-off-by: Esben Haabendal <eha@deif.com>
>>> ---
>>> drivers/net/phy/dp83640.c | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
>>> index 654f42d00092..48403170096a 100644
>>> --- a/drivers/net/phy/dp83640.c
>>> +++ b/drivers/net/phy/dp83640.c
>>> @@ -1207,6 +1207,22 @@ static void dp83640_remove(struct phy_device *phydev)
>>> kfree(dp83640);
>>> }
>>>
>>> +static int dp83640_soft_reset(struct phy_device *phydev)
>>> +{
>>> + int ret;
>>> +
>>> + ret = genphy_soft_reset(phydev);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + /* From DP83640 datasheet: "Software driver code must wait 3 us
>>> + * following a software reset before allowing further serial MII
>>> + * operations with the DP83640." */
>>> + udelay(3);
>>
>> Hi Esben
>>
>> The accuracy of udelay() is not guaranteed. So you probably want to be
>> a bit pessimistic, and use 10.
Ok, will do.
/Esben
^ permalink raw reply
* Re: [PATCH 3/9] kbuild: Do not pass arguments to link-vmlinux.sh
From: Jiri Olsa @ 2018-04-06 16:54 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, lkml, netdev,
Linux Kbuild mailing list, Quentin Monnet, Eugene Syromiatnikov,
Jiri Benc, Stanislav Kozina, Jerome Marchand,
Arnaldo Carvalho de Melo, Michal Marek, Jiri Kosina
In-Reply-To: <CAK7LNATU9_zBH9DpayNOCGsFVL0K+Y8Np0z7wxFuwO3Cf1K_tg@mail.gmail.com>
On Fri, Apr 06, 2018 at 09:59:59AM +0900, Masahiro Yamada wrote:
> 2018-04-06 3:59 GMT+09:00 Jiri Olsa <jolsa@redhat.com>:
> > On Fri, Apr 06, 2018 at 12:50:00AM +0900, Masahiro Yamada wrote:
> >> 2018-04-06 0:16 GMT+09:00 Jiri Olsa <jolsa@kernel.org>:
> >> > There's no need to pass LD* arguments to link-vmlinux.sh,
> >> > because they are passed as variables. The only argument
> >> > the link-vmlinux.sh supports is the 'clean' argument.
> >> >
> >> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >> > ---
> >>
> >> Wrong.
> >>
> >> $(LD) $(LDFLAGS) $(LDFLAGS_vmlinux)
> >> exist here so that any change in them
> >> invokes scripts/linkk-vmlinux.sh
> >
> > sry, I can't see that.. but it's just a side fix,
> > which is actually not needed for the rest
> >
> > I'll check on more and address this separately
>
>
> The link command is recorded in .vmlinux.cmd
i see.. just for the sake command line,
thanks for explanation
jirka
^ permalink raw reply
* Re: TCP one-by-one acking - RFC interpretation question
From: Eric Dumazet @ 2018-04-06 16:49 UTC (permalink / raw)
To: Michal Kubecek, Eric Dumazet; +Cc: netdev, Yuchung Cheng, Neal Cardwell
In-Reply-To: <20180406150345.i3t7cu5g6dmoc3io@unicorn.suse.cz>
Cc Neal and Yuchung if they missed this thread.
On 04/06/2018 08:03 AM, Michal Kubecek wrote:
> On Fri, Apr 06, 2018 at 05:01:29AM -0700, Eric Dumazet wrote:
>>
>>
>> On 04/06/2018 03:05 AM, Michal Kubecek wrote:
>>> Hello
>>>
>>> I encountered a strange behaviour of some (non-linux) TCP stack which
>>> I believe is incorrect but support engineers from the company producing
>>> it claim is OK.
>>>
>>> Assume a client (sender, Linux 4.4 kernel) sends a stream of MSS sized
>>> segments but segments 2, 4 and 6 do not reach the server (receiver):
>>>
>>> ACK SAK SAK SAK
>>> +-------+-------+-------+-------+-------+-------+-------+
>>> | 1 | 2 | 3 | 4 | 5 | 6 | 7 |
>>> +-------+-------+-------+-------+-------+-------+-------+
>>> 34273 35701 37129 38557 39985 41413 42841 44269
>>>
>>> When segment 2 is retransmitted after RTO timeout, normal response would
>>> be ACK-ing segment 3 (38557) with SACK for 5 and 7 (39985-41413 and
>>> 42841-44269).
>>>
>>> However, this server stack responds with two separate ACKs:
>>>
>>> - ACK 37129, SACK 37129-38557 39985-41413 42841-44269
>>> - ACK 38557, SACK 39985-41413 42841-44269
>>
>> Hmmm... Yes this seems very very wrong and lazy.
>>
>> Have you verified behavior of more recent linux kernel to such threats ?
>
> No, unfortunately the problem was only encountered by our customer in
> production environment (they tried to reproduce in a test lab but no
> luck). They are running backups to NFS server and it happens from time
> to time (in the order of hours, IIUC). So it would be probably hard to
> let them try with more recent kernel.
>
> On the other hand, they reported that SLE11 clients (kernel 3.0) do not
> run into this kind of problem. It was originally reported as a
> a regression on migration from SLE11-SP4 (3.0 kernel) to SLE12-SP2 (4.4
> kernel) and the problem was reported as "SLE12-SP2 is ignoring dupacks"
> (which seems to be mostly caused by the switch to RACK).
>
> It also seems that part of the problem is specific packet loss pattern
> where at some point, many packets are lost in "every second" pattern.
> The customer finally started to investigate this problem and it seems it
> has something to do with their bonding setup (they provided no details,
> my guess is packets are divided over two paths and one of them fails).
>
>> packetdrill test would be relatively easy to write.
>
> I'll try but I have very little experience with writing packetdrill
> scripts so it will probably take some time.
>
>> Regardless of this broken alien stack, we might be able to work around
>> this faster than the vendor is able to fix and deploy a new stack.
>>
>> ( https://en.wikipedia.org/wiki/Robustness_principle )
>> Be conservative in what you do, be liberal in what you accept from
>> others...
>
> I was thinking about this a bit. "Fixing" the acknowledgment number
> could do the trick but it doesn't feel correct. We might use the fact
> that TSecr of both ACKs above matches TSval of the retransmission which
> triggered them so that RTT calculated from timestamp would be the right
> one. So perhaps something like "prefer timestamp RTT if measured RTT
> seems way too off". But I'm not sure if it couldn't break other use
> cases where (high) measured RTT is actually correct, rather than (low)
> timestamp RTT.
>
> Michal Kubecek
>
^ permalink raw reply
* Re: [PATCH net-next] netns: filter uevents correctly
From: Eric W. Biederman @ 2018-04-06 16:48 UTC (permalink / raw)
To: Christian Brauner
Cc: Kirill Tkhai, davem, gregkh, netdev, linux-kernel, avagin, serge
In-Reply-To: <20180406160757.GA16281@gmail.com>
Christian Brauner <christian.brauner@canonical.com> writes:
>> At a practical level there should be no receivers. Plus performance
>> issues. At least my memory is that any unprivileged user on the system
>> is allowed to listen to those events.
>
> Any unprivileged user is allowed to listen to uevents if they have
> net_broadcast in the user namespace the uevent socket was opened in;
> unless I'm misreading.
I believe you are.
This code in do_one_broadcast.
if (!net_eq(sock_net(sk), p->net)) {
if (!(nlk->flags & NETLINK_F_LISTEN_ALL_NSID))
return;
if (!peernet_has_id(sock_net(sk), p->net))
return;
if (!file_ns_capable(sk->sk_socket->file, p->net->user_ns,
CAP_NET_BROADCAST))
return;
}
Used to just be:
if (!net_eq(sock_net(sk), p->net))
return;
Which makes sense when you have a shared hash table and a shared mc_list
between network namespaces.
There is a non-container use of network namespaces where you just need
different contexts were ip addresses can overlap etc. In that
configuration where a single program is mananging multiple network
namespaces being able to listen to rtnetlink events in all of them is an
advantage.
For that case a special socket option NETLINK_F_LISTEN_ALL_NSID was
added that allowed one socket to listen for events from multiple network
namespaces.
If we rework the code in af_netlink.c that matters. However for just
understanding uevents you can assume there are no sockets with
NETLINK_F_LISTEN_ALL_NSID set.
Eric
^ permalink raw reply
* Re: [PATCH net-next] netns: filter uevents correctly
From: Christian Brauner @ 2018-04-06 16:07 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Kirill Tkhai, davem, gregkh, netdev, linux-kernel, avagin, serge
In-Reply-To: <874lko2y22.fsf@xmission.com>
On Fri, Apr 06, 2018 at 09:45:41AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner@canonical.com> writes:
>
> > On Thu, Apr 05, 2018 at 10:59:49PM -0500, Eric W. Biederman wrote:
> >> Christian Brauner <christian.brauner@canonical.com> writes:
> >>
> >> > On Thu, Apr 05, 2018 at 05:26:59PM +0300, Kirill Tkhai wrote:
> >> >> On 05.04.2018 17:07, Christian Brauner wrote:
> >> >> > On Thu, Apr 05, 2018 at 04:01:03PM +0300, Kirill Tkhai wrote:
> >> >> >> On 04.04.2018 22:48, Christian Brauner wrote:
> >> >> >>> commit 07e98962fa77 ("kobject: Send hotplug events in all network namespaces")
> >> >> >>>
> >> >> >>> enabled sending hotplug events into all network namespaces back in 2010.
> >> >> >>> Over time the set of uevents that get sent into all network namespaces has
> >> >> >>> shrunk. We have now reached the point where hotplug events for all devices
> >> >> >>> that carry a namespace tag are filtered according to that namespace.
> >> >> >>>
> >> >> >>> Specifically, they are filtered whenever the namespace tag of the kobject
> >> >> >>> does not match the namespace tag of the netlink socket. One example are
> >> >> >>> network devices. Uevents for network devices only show up in the network
> >> >> >>> namespaces these devices are moved to or created in.
> >> >> >>>
> >> >> >>> However, any uevent for a kobject that does not have a namespace tag
> >> >> >>> associated with it will not be filtered and we will *try* to broadcast it
> >> >> >>> into all network namespaces.
> >> >> >>>
> >> >> >>> The original patchset was written in 2010 before user namespaces were a
> >> >> >>> thing. With the introduction of user namespaces sending out uevents became
> >> >> >>> partially isolated as they were filtered by user namespaces:
> >> >> >>>
> >> >> >>> net/netlink/af_netlink.c:do_one_broadcast()
> >> >> >>>
> >> >> >>> if (!net_eq(sock_net(sk), p->net)) {
> >> >> >>> if (!(nlk->flags & NETLINK_F_LISTEN_ALL_NSID))
> >> >> >>> return;
> >> >> >>>
> >> >> >>> if (!peernet_has_id(sock_net(sk), p->net))
> >> >> >>> return;
> >> >> >>>
> >> >> >>> if (!file_ns_capable(sk->sk_socket->file, p->net->user_ns,
> >> >> >>> CAP_NET_BROADCAST))
> >> >> >>> j return;
> >> >> >>> }
> >> >> >>>
> >> >> >>> The file_ns_capable() check will check whether the caller had
> >> >> >>> CAP_NET_BROADCAST at the time of opening the netlink socket in the user
> >> >> >>> namespace of interest. This check is fine in general but seems insufficient
> >> >> >>> to me when paired with uevents. The reason is that devices always belong to
> >> >> >>> the initial user namespace so uevents for kobjects that do not carry a
> >> >> >>> namespace tag should never be sent into another user namespace. This has
> >> >> >>> been the intention all along. But there's one case where this breaks,
> >> >> >>> namely if a new user namespace is created by root on the host and an
> >> >> >>> identity mapping is established between root on the host and root in the
> >> >> >>> new user namespace. Here's a reproducer:
> >> >> >>>
> >> >> >>> sudo unshare -U --map-root
> >> >> >>> udevadm monitor -k
> >> >> >>> # Now change to initial user namespace and e.g. do
> >> >> >>> modprobe kvm
> >> >> >>> # or
> >> >> >>> rmmod kvm
> >> >> >>>
> >> >> >>> will allow the non-initial user namespace to retrieve all uevents from the
> >> >> >>> host. This seems very anecdotal given that in the general case user
> >> >> >>> namespaces do not see any uevents and also can't really do anything useful
> >> >> >>> with them.
> >> >> >>>
> >> >> >>> Additionally, it is now possible to send uevents from userspace. As such we
> >> >> >>> can let a sufficiently privileged (CAP_SYS_ADMIN in the owning user
> >> >> >>> namespace of the network namespace of the netlink socket) userspace process
> >> >> >>> make a decision what uevents should be sent.
> >> >> >>>
> >> >> >>> This makes me think that we should simply ensure that uevents for kobjects
> >> >> >>> that do not carry a namespace tag are *always* filtered by user namespace
> >> >> >>> in kobj_bcast_filter(). Specifically:
> >> >> >>> - If the owning user namespace of the uevent socket is not init_user_ns the
> >> >> >>> event will always be filtered.
> >> >> >>> - If the network namespace the uevent socket belongs to was created in the
> >> >> >>> initial user namespace but was opened from a non-initial user namespace
> >> >> >>> the event will be filtered as well.
> >> >> >>> Put another way, uevents for kobjects not carrying a namespace tag are now
> >> >> >>> always only sent to the initial user namespace. The regression potential
> >> >> >>> for this is near to non-existent since user namespaces can't really do
> >> >> >>> anything with interesting devices.
> >> >> >>>
> >> >> >>> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> >> >> >>> ---
> >> >> >>> lib/kobject_uevent.c | 10 +++++++++-
> >> >> >>> 1 file changed, 9 insertions(+), 1 deletion(-)
> >> >> >>>
> >> >> >>> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> >> >> >>> index 15ea216a67ce..cb98cddb6e3b 100644
> >> >> >>> --- a/lib/kobject_uevent.c
> >> >> >>> +++ b/lib/kobject_uevent.c
> >> >> >>> @@ -251,7 +251,15 @@ static int kobj_bcast_filter(struct sock *dsk, struct sk_buff *skb, void *data)
> >> >> >>> return sock_ns != ns;
> >> >> >>> }
> >> >> >>>
> >> >> >>> - return 0;
> >> >> >>> + /*
> >> >> >>> + * The kobject does not carry a namespace tag so filter by user
> >> >> >>> + * namespace below.
> >> >> >>> + */
> >> >> >>> + if (sock_net(dsk)->user_ns != &init_user_ns)
> >> >> >>> + return 1;
> >> >> >>> +
> >> >> >>> + /* Check if socket was opened from non-initial user namespace. */
> >> >> >>> + return sk_user_ns(dsk) != &init_user_ns;
> >> >> >>> }
> >> >> >>> #endif
> >> >> >>
> >> >> >> So, this prohibits to listen events of all devices except network-related
> >> >> >> in containers? If it's so, I don't think it's a good solution. Uevents is not
> >> >> >
> >> >> > No, this is not correct: As it is right now *without my patch* no
> >> >> > non-initial user namespace is receiving *any uevents* but those
> >> >> > specifically namespaced such as those for network devices. This patch
> >> >> > doesn't change that at all. The commit message outlines this in detail
> >> >> > how this comes about.
> >> >> > There is only one case where this currently breaks and this is as I
> >> >> > outlined explicitly in my commit message when you create a new user
> >> >> > namespace and map container(0) -> host(0). This patch fixes this.
> >> >>
> >> >> Could you please point the place, where non-initial user namespaces are filtered?
> >> >> I only see the kobj_bcast_filter() logic, and it used to return 0, which means "accepted".
> >> >> Now it will return 1 sometimes.
> >> >
> >> > Oh sure, it's in the commit message though. The callchain is
> >> > lib/kobject_uevent.c:kobject_uevent_net_broadcast() ->
> >> > nnet/netlink/af_netlink.c:netlink_broadcast_filtered() ->
> >> > net/netlink/af_netlink.c:do_one_broadcast():
> >> >
> >> > This codepiece will check whether the openened socket holds
> >> > CAP_NET_BROADCAST in the user namespace of the target network namespace
> >> > which it won't because we don't have device namespaces and all devices
> >> > belong to the initial set of namespaces.
> >> >
> >> > if (!file_ns_capable(sk->sk_socket->file, p->net->user_ns,
> >> > CAP_NET_BROADCAST))
> >> > j return;
> >> >
> >>
> >> The above that only applies if someone has set NETLINK_F_LISTEN_ALL_NSID
> >> on their socket and has had someone with the appropriate privileges
> >> assign a peerrnetid.
> >>
> >> All of which is to say that file_ns_capable is not nearly as applicable
> >> as it might be, and if you can pass the other two checks I think it is
> >> pointless (because the peernet attributes are not generated for
> >> kobj_uevents) but valid to receive events from outside your network
> >> namespace.
> >>
> >>
> >> I might be missing something but I don't see anything excluding network
> >> namespaces owned by !init_user_ns excluded from the kobject_uevent
> >> logic.
> >>
> >> The uevent_sock_list has one entry per network namespace. And
> >> kobject_uevent_net_broacast appears to walk each one.
> >
> > Yeah, it definitely does.
> >
> >>
> >> I had a memory of filtering uevent messages and I had a memory
> >> that the netlink_has_listeners had a per network namespace effect.
> >> Neither seems true from my inspection of the code tonight.
> >
> > Yeah, I drew the same conclusion.
> >
> >>
> >> If we are not filtering ordinary uevents at least at the user namespace
> >> level that does seem to be at least a nuisance.
> >
> > This patch would filter based on user namespace and bump it from "we're
> > accidently doing the right thing" to "we're doing the right on purpose"
> > and without accidently leaking uevents in some corner cases.
> > Using kobj_bcast_filter() for this seems like the right thing to do.
> > This sounds like you don't disagree with the approach I'm taking here?
>
> How are we accidentally filtering? If we can not describe how the code
> currently works to achieve something we don't understand it well enough
> to change it.
I absolutely agree and without doing a lot of semantics here I just
meant that so far this all worked on accident that doesn't presuppose
that we understand why it worked.
>
>
>
> > On a sidenote, it also very much feels like we're leaking information if
> > we're not filtering based on user namespaces on purpose. Non-initial
> > user namespaces should not be able to receive information about device
> > attribues simply via netlink uevent messages. At least not *trivially*
> > [1] by opening a netlink uevent socket.
>
> This is not at all about isolation. Devices don't belong to user
Sure, but there's still a difference between an unprivileged host user
listening to uevents and user namespaces that they might have delegated.
> namespaces. This is about it being useless/silly to send events
> to those sockets as the receiver could not do anything with the uevent.
Yes, indeed unless a sufficiently privileged process explicitly decides
that a given uevent should be sent.
> At a practical level there should be no receivers. Plus performance
> issues. At least my memory is that any unprivileged user on the system
> is allowed to listen to those events.
Any unprivileged user is allowed to listen to uevents if they have
net_broadcast in the user namespace the uevent socket was opened in;
unless I'm misreading.
>
> At least at one point udev listening and reacting to events in every
> network namespace in containers was a significant slow down on the
> system.
Sure, because we're holding a global lock on the list of all uevent
sockets and then go on to walk all of mc_list for each of those sockets
when broadcasting to guarantee that uevents are correctly ordered:
mutex_lock(&uevent_sock_mutex);
/* we will send an event, so request a new sequence number */
retval = add_uevent_var(env, "SEQNUM=%llu", (unsigned long long)++uevent_seqnum);
if (retval) {
mutex_unlock(&uevent_sock_mutex);
goto exit;
}
retval = kobject_uevent_net_broadcast(kobj, env, action_string,
devpath);
mutex_unlock(&uevent_sock_mutex);
which means that the time it takes for uevents to be sent out increases
drastically the more listeners you have in more network namespaces. Even
for just one network namespace you have quite a lot:
ss -e -f netlink -a | grep uevent
then just look at those that you can clearly associate with a single
program I typically get ~20 listeners.
>
> The report was that adding netlink_has_listeners greatly sped up uevent
> sending. That testing appears faulty. I think the real gain was
> userspace not listening to uevents in every container.
Banal point but the truth is probably not userspace changes in general
but that most massive container deployments are app containers. So
there's no udevd.
>
> Which means uevent injection for containers has some real technical
> challenges to overcome before it can be wide spread.
>
> >> Christian can you dig a little deeper into this. I have a feeling that
> >> there are some real efficiency improvements that we could make to
> >> kobject_uevent_net_broadcast if nothing else.
> >
> > Yeah, for sure! I already started doing this. This patch here is
> > basically a preparatory patch for that work which is on my todo.
>
> Limiting the network namespaces on uevent_sock_list to just network
> namespaces owned by the initial user namespaces would be a lot better
> than your patch.
I had this idea as well but I decided against it because of namespace
tag carrying kobjects. The only kobjects that fall into this category
are network devices so I need to think through this again and more
thoroughly.
>
>
> At a practical level I am concerned what will happen when we stand up a
> uevent listener aka udev in say 100 or maybe 1000 unprivileged
> containers on a system. History suggests the current data structures
> won't scale to the problem, and the fixes that were put in place kernel
> side only did not change anything. It was only the userspace fixes
> that made a difference.
>
> Which suggests either improving the isolation between network namespaces
> for netlink multicast sockets or putting:
>
>
> if (!net_eq(sock_net(sk), p->net)) {
> if (!(nlk->flags & NETLINK_F_LISTEN_ALL_NSID))
> return;
>
> if (!peernet_has_id(sock_net(sk), p->net))
> return;
>
> if (!file_ns_capable(sk->sk_socket->file, p->net->user_ns,
> CAP_NET_BROADCAST))
> return;
> }
>
> In a filter and having an appropriate filter so that
> netlink_broadcast_filtered only needs to be called once from
> kobject_uevent_net_broadcast.
Maybe, but I'd like to try and find a solution that let's us wipe the
uevent socket list.
>
> It is more difficult but in practice I expect we have far more to gain
> by fixing the mc_list and the netlink_has_listeners function to be per
> network namespace basis. Handling the NETLINK_F_LISTEN_ALL_NSID will
> be trickier in that case but the current semantics appear correct
> for that case.
Yeah, I'll dig into this.
>
> But before we do anything we have to test and see if the assertion
> that we don't make it to netlink listeners in network namespaces
> that are outside of the init_user_ns is true. If it is true we need to
> find the code that makes it true.
Yeah, I'm on this as well.
Thanks!
Christian
^ permalink raw reply
* Re: [net 0/2][pull request] Intel Wired LAN Driver Updates 2018-04-06
From: David Miller @ 2018-04-06 15:39 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: netdev, nhorman, sassmann, jogreene
In-Reply-To: <20180406153630.9321-1-jeffrey.t.kirsher@intel.com>
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 6 Apr 2018 08:36:28 -0700
> This series contains a couple of fixes for the new ice driver.
>
> Wei Yongjun fixes the return error code for error case during init.
>
> Anirudh fixes the incorrect use of ARRAY_SIZE() in the ice ethtool code
> and fixed "for" loop calculations.
Pulled, thanks Jeff.
^ permalink raw reply
* [net 1/2] ice: Fix error return code in ice_init_hw()
From: Jeff Kirsher @ 2018-04-06 15:36 UTC (permalink / raw)
To: davem; +Cc: Wei Yongjun, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <20180406153630.9321-1-jeffrey.t.kirsher@intel.com>
From: Wei Yongjun <weiyongjun1@huawei.com>
Fix to return error code ICE_ERR_NO_MEMORY from the alloc error
handling case instead of 0, as done elsewhere in this function.
Fixes: dc49c7723676 ("ice: Get MAC/PHY/link info and scheduler topology")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Acked-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ice/ice_common.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 385f5d425d19..21977ec984c4 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -468,8 +468,10 @@ enum ice_status ice_init_hw(struct ice_hw *hw)
mac_buf_len = sizeof(struct ice_aqc_manage_mac_read_resp);
mac_buf = devm_kzalloc(ice_hw_to_dev(hw), mac_buf_len, GFP_KERNEL);
- if (!mac_buf)
+ if (!mac_buf) {
+ status = ICE_ERR_NO_MEMORY;
goto err_unroll_fltr_mgmt_struct;
+ }
status = ice_aq_manage_mac_read(hw, mac_buf, mac_buf_len, NULL);
devm_kfree(ice_hw_to_dev(hw), mac_buf);
--
2.14.3
^ permalink raw reply related
* [net 0/2][pull request] Intel Wired LAN Driver Updates 2018-04-06
From: Jeff Kirsher @ 2018-04-06 15:36 UTC (permalink / raw)
To: davem; +Cc: Jeff Kirsher, netdev, nhorman, sassmann, jogreene
This series contains a couple of fixes for the new ice driver.
Wei Yongjun fixes the return error code for error case during init.
Anirudh fixes the incorrect use of ARRAY_SIZE() in the ice ethtool code
and fixed "for" loop calculations.
The following are changes since commit 3239534a79ee6f20cffd974173a1e62e0730e8ac:
net/sched: fix NULL dereference in the error path of tcf_bpf_init()
and are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net-queue 100GbE
Anirudh Venkataramanan (1):
ice: Bug fixes in ethtool code
Wei Yongjun (1):
ice: Fix error return code in ice_init_hw()
drivers/net/ethernet/intel/ice/ice_common.c | 4 +++-
drivers/net/ethernet/intel/ice/ice_ethtool.c | 4 ++--
2 files changed, 5 insertions(+), 3 deletions(-)
--
2.14.3
^ permalink raw reply
* [net 2/2] ice: Bug fixes in ethtool code
From: Jeff Kirsher @ 2018-04-06 15:36 UTC (permalink / raw)
To: davem
Cc: Anirudh Venkataramanan, netdev, nhorman, sassmann, jogreene,
Jeff Kirsher
In-Reply-To: <20180406153630.9321-1-jeffrey.t.kirsher@intel.com>
From: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
1) Return correct size from ice_get_regs_len.
2) Fix incorrect use of ARRAY_SIZE in ice_get_regs.
Fixes: fcea6f3da546 (ice: Add stats and ethtool support)
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ethtool.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 186764a5c263..1db304c01d10 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -156,7 +156,7 @@ ice_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo)
static int ice_get_regs_len(struct net_device __always_unused *netdev)
{
- return ARRAY_SIZE(ice_regs_dump_list);
+ return sizeof(ice_regs_dump_list);
}
static void
@@ -170,7 +170,7 @@ ice_get_regs(struct net_device *netdev, struct ethtool_regs *regs, void *p)
regs->version = 1;
- for (i = 0; i < ARRAY_SIZE(ice_regs_dump_list) / sizeof(u32); ++i)
+ for (i = 0; i < ARRAY_SIZE(ice_regs_dump_list); ++i)
regs_buf[i] = rd32(hw, ice_regs_dump_list[i]);
}
--
2.14.3
^ permalink raw reply related
* Re: [PATCH net 1/3] lan78xx: PHY DSP registers initialization to address EEE link drop issues with long cables
From: David Miller @ 2018-04-06 15:21 UTC (permalink / raw)
To: andrew; +Cc: raghuramchary.jallipalli, netdev, unglinuxdriver, woojung.huh
In-Reply-To: <20180406144342.GN17495@lunn.ch>
From: Andrew Lunn <andrew@lunn.ch>
Date: Fri, 6 Apr 2018 16:43:42 +0200
> On Fri, Apr 06, 2018 at 11:42:02AM +0530, Raghuram Chary J wrote:
>> The patch is to configure DSP registers of PHY device
>> to handle Gbe-EEE failures with >40m cable length.
>>
>> Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
>> Signed-off-by: Raghuram Chary J <raghuramchary.jallipalli@microchip.com>
>> ---
>> drivers/net/phy/microchip.c | 123 ++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/microchipphy.h | 8 +++
>> 2 files changed, 130 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c
>> index 0f293ef28935..174ae9808722 100644
>> --- a/drivers/net/phy/microchip.c
>> +++ b/drivers/net/phy/microchip.c
>> @@ -20,6 +20,7 @@
>> #include <linux/ethtool.h>
>> #include <linux/phy.h>
>> #include <linux/microchipphy.h>
>> +#include <linux/delay.h>
>>
>> #define DRIVER_AUTHOR "WOOJUNG HUH <woojung.huh@microchip.com>"
>> #define DRIVER_DESC "Microchip LAN88XX PHY driver"
>> @@ -66,6 +67,107 @@ static int lan88xx_suspend(struct phy_device *phydev)
>> return 0;
>> }
>>
>> +static void lan88xx_TR_reg_set(struct phy_device *phydev, u16 regaddr,
>> + u32 data)
>> +{
>> + int val;
>> + u16 buf;
>> +
>> + /* Get access to token ring page */
>> + phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
>> + LAN88XX_EXT_PAGE_ACCESS_TR);
>
> Hi Raghuram
>
> You might want to look at phy_read_paged(), phy_write_paged(), etc.
>
> There can be race conditions with paged access.
Yep, so something like:
static void lan88xx_TR_reg_set(struct phy_device *phydev, u16 regaddr,
u32 data)
{
int save_page, val;
u16 buf;
save_page = phy_save_page(phydev);
phy_write_paged(phydev, LAN88XX_EXT_PAGE_ACCESS_TR,
LAN88XX_EXT_PAGE_TR_LOW_DATA, (data & 0xFFFF));
phy_write_paged(phydev, LAN88XX_EXT_PAGE_ACCESS_TR,
LAN88XX_EXT_PAGE_TR_HIGH_DATA,
(data & 0x00FF0000) >> 16);
/* Config control bits [15:13] of register */
buf = (regaddr & ~(0x3 << 13));/* Clr [14:13] to write data in reg */
buf |= 0x8000; /* Set [15] to Packet transmit */
phy_write_paged(phydev, LAN88XX_EXT_PAGE_ACCESS_TR,
LAN88XX_EXT_PAGE_TR_CR, buf);
usleep_range(1000, 2000);/* Wait for Data to be written */
val = phy_read_paged(phydev, LAN88XX_EXT_PAGE_ACCESS_TR,
LAN88XX_EXT_PAGE_TR_CR);
if (!(val & 0x8000))
pr_warn("TR Register[0x%X] configuration failed\n", regaddr);
phy_restore_page(phydev, save_page, 0);
}
Since PHY accesses and thus things like phy_save_page() can fail, the
return type of this function should be changed to 'int' and some error
checking should be added.
^ permalink raw reply
* Re: [PATCH v3] net: thunderx: rework mac addresses list to u64 array
From: Vadim Lomovtsev @ 2018-04-06 15:16 UTC (permalink / raw)
To: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel
Cc: dnelson, Vadim Lomovtsev
In-Reply-To: <20180406140443.15181-1-Vadim.Lomovtsev@caviumnetworks.com>
Self-NACK here, because of https://lkml.org/lkml/2018/4/6/724
Sorry for noise.
Vadim
On Fri, Apr 06, 2018 at 07:04:43AM -0700, Vadim Lomovtsev wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
>
> It is too expensive to pass u64 values via linked list, instead
> allocate array for them by overall number of mac addresses from netdev.
>
> This eventually removes multiple kmalloc() calls, aviod memory
> fragmentation and allow to put single null check on kmalloc
> return value in order to prevent a potential null pointer dereference.
>
> Addresses-Coverity-ID: 1467429 ("Dereference null return value")
> Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback implementation for VF")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@cavium.com>
> ---
> Changes from v1 to v2:
> - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
> Changes from v2 to v3:
> - update commit description with 'Reported-by: Dan Carpenter';
> - update size calculations for mc list to offsetof() call
> instead of explicit arithmetic;
> ---
> drivers/net/ethernet/cavium/thunder/nic.h | 7 +-----
> drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +++++++++---------------
> 2 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> index 5fc46c5a4f36..448d1fafc827 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>
> struct cavium_ptp;
>
> -struct xcast_addr {
> - struct list_head list;
> - u64 addr;
> -};
> -
> struct xcast_addr_list {
> - struct list_head list;
> int count;
> + u64 mc[];
> };
>
> struct nicvf_work {
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index 1e9a31fef729..7d9e58533a83 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + u8 idx = 0;
>
> if (!vf_work)
> return;
> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> /* check if we have any specific MACs to be added to PF DMAC filter */
> if (vf_work->mc) {
> /* now go through kernel list of MACs and add them one by one */
> - list_for_each_entry_safe(xaddr, next,
> - &vf_work->mc->list, list) {
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
> mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
> - mbx.xcast.data.mac = xaddr->addr;
> + mbx.xcast.data.mac = vf_work->mc->mc[idx];
> nicvf_send_msg_to_pf(nic, &mbx);
> -
> - /* after receiving ACK from PF release memory */
> - list_del(&xaddr->list);
> - kfree(xaddr);
> - vf_work->mc->count--;
> }
> kfree(vf_work->mc);
> }
> @@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
> mode |= BGX_XCAST_MCAST_FILTER;
> /* here we need to copy mc addrs */
> if (netdev_mc_count(netdev)) {
> - struct xcast_addr *xaddr;
> -
> - mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
> - INIT_LIST_HEAD(&mc_list->list);
> + mc_list = kmalloc(offsetof(typeof(*mc_list),
> + mc[netdev_mc_count(netdev)]),
> + GFP_ATOMIC);
> + if (unlikely(!mc_list))
> + return;
> + mc_list->count = 0;
> netdev_hw_addr_list_for_each(ha, &netdev->mc) {
> - xaddr = kmalloc(sizeof(*xaddr),
> - GFP_ATOMIC);
> - xaddr->addr =
> + mc_list->mc[mc_list->count] =
> ether_addr_to_u64(ha->addr);
> - list_add_tail(&xaddr->list,
> - &mc_list->list);
> mc_list->count++;
> }
> }
> --
> 2.14.3
>
^ permalink raw reply
* Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array
From: Vadim Lomovtsev @ 2018-04-06 15:14 UTC (permalink / raw)
To: David Miller
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, dnelson, gustavo, Vadim.Lomovtsev
In-Reply-To: <20180406.110603.978796669734920248.davem@davemloft.net>
On Fri, Apr 06, 2018 at 11:06:03AM -0400, David Miller wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> Date: Fri, 6 Apr 2018 04:14:25 -0700
>
> > diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> > index 5fc46c5a4f36..448d1fafc827 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nic.h
> > +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> > @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
> >
> > struct cavium_ptp;
> >
> > -struct xcast_addr {
> > - struct list_head list;
> > - u64 addr;
> > -};
> > -
> > struct xcast_addr_list {
> > - struct list_head list;
> > int count;
> > + u64 mc[];
> > };
> >
> > struct nicvf_work {
> > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > index 1e9a31fef729..a26d8bc92e01 100644
> > --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > - struct xcast_addr *xaddr, *next;
> > + u8 idx = 0;
> ^^^^^^^^^^^
>
> >
> > if (!vf_work)
> > return;
> > @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > /* check if we have any specific MACs to be added to PF DMAC filter */
> > if (vf_work->mc) {
> > /* now go through kernel list of MACs and add them one by one */
> > - list_for_each_entry_safe(xaddr, next,
> > - &vf_work->mc->list, list) {
> > + for (idx = 0; idx < vf_work->mc->count; idx++) {
>
> vf_work->mx->count is an 'int' therefore 'idx' should be declared 'int' as well,
> not a 'u8'.
My bad, sorry.
Will post v4 shortly then.
WBR,
Vadim
^ permalink raw reply
* Re: [PATCH] dp83640: Ensure against premature access to PHY registers after reset
From: David Miller @ 2018-04-06 15:13 UTC (permalink / raw)
To: andrew
Cc: esben.haabendal, netdev, eha, richardcochran, f.fainelli,
linux-kernel
In-Reply-To: <20180406141410.GI17495@lunn.ch>
From: Andrew Lunn <andrew@lunn.ch>
Date: Fri, 6 Apr 2018 16:14:10 +0200
> On Fri, Apr 06, 2018 at 04:05:40PM +0200, Esben Haabendal wrote:
>> From: Esben Haabendal <eha@deif.com>
>>
>> Signed-off-by: Esben Haabendal <eha@deif.com>
>> ---
>> drivers/net/phy/dp83640.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
>> index 654f42d00092..48403170096a 100644
>> --- a/drivers/net/phy/dp83640.c
>> +++ b/drivers/net/phy/dp83640.c
>> @@ -1207,6 +1207,22 @@ static void dp83640_remove(struct phy_device *phydev)
>> kfree(dp83640);
>> }
>>
>> +static int dp83640_soft_reset(struct phy_device *phydev)
>> +{
>> + int ret;
>> +
>> + ret = genphy_soft_reset(phydev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* From DP83640 datasheet: "Software driver code must wait 3 us
>> + * following a software reset before allowing further serial MII
>> + * operations with the DP83640." */
>> + udelay(3);
>
> Hi Esben
>
> The accuracy of udelay() is not guaranteed. So you probably want to be
> a bit pessimistic, and use 10.
Agreed.
^ permalink raw reply
* Re: [PATCH net-next 0/5] net: stmmac: Stop using hard-coded callbacks
From: David Miller @ 2018-04-06 15:12 UTC (permalink / raw)
To: Jose.Abreu; +Cc: netdev, Joao.Pinto, peppe.cavallaro, alexandre.torgue
In-Reply-To: <cover.1523019346.git.joabreu@synopsys.com>
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Fri, 6 Apr 2018 14:08:14 +0100
> This a starting point for a cleanup and re-organization of stmmac.
>
> In this series we stop using hard-coded callbacks along the code and use
> instead helpers which are defined in a single place ("hwif.h").
>
> This brings several advantages:
> 1) Less typing :)
> 2) Guaranteed function pointer check
> 3) More flexibility
>
> By 2) we stop using the repeated pattern of:
> if (priv->hw->mac->some_func)
> priv->hw->mac->some_func(...)
>
> I didn't check but I expect the final .ko will be bigger with this series
> because *all* of function pointers are checked.
>
> Anyway, I hope this can make the code more readable and more flexible now.
The net-next tree is closed, please resubmit this series when it opens
back up.
Thank you.
^ permalink raw reply
* Re: [RFC 0/9] bpf: Add buildid check support
From: Jiri Olsa @ 2018-04-06 15:07 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, lkml, netdev,
linux-kbuild, Quentin Monnet, Eugene Syromiatnikov, Jiri Benc,
Stanislav Kozina, Jerome Marchand, Arnaldo Carvalho de Melo,
Masahiro Yamada, Michal Marek, Jiri Kosina
In-Reply-To: <20180406013719.ekptkxbwzpjeaanu@ast-mbp.dhcp.thefacebook.com>
On Thu, Apr 05, 2018 at 06:37:23PM -0700, Alexei Starovoitov wrote:
> On Thu, Apr 05, 2018 at 05:16:36PM +0200, Jiri Olsa wrote:
> > hi,
> > eBPF programs loaded for kprobes are allowed to read kernel
> > internal structures. We check the provided kernel version
> > to ensure that the program is loaded for the proper kernel.
> >
> > The problem is that the version check is not enough, because
> > it only follows the version setup from kernel's Makefile.
> > However, the internal kernel structures change based on the
> > .config data, so in practise we have different kernels with
> > same version.
> >
> > The eBPF kprobe program thus then get loaded for different
> > kernel than it's been built for, get wrong data (silently)
> > and provide misleading output.
> >
> > This patchset implements additional check in eBPF loading code
> > on provided build ID (from kernel's elf image, .notes section
> > GNU build ID) to ensure we load the eBPF program on correct
> > kernel.
> >
> > Also available in here (based on bpf-next/master):
> > https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > bpf/checksum
> >
> > This patchset consists of several changes:
> >
> > - adding CONFIG_BUILDID_H option that instructs the build
> > to generate uapi header file with build ID data, that
> > will be included by eBPF program
> >
> > - adding CONFIG_BPF_BUILDID_CHECK option and new bpf_attr
> > field to allow build ID checking when loading the eBPF
> > program
> >
> > - changing libbpf to read and pass build ID to the kernel
> >
> > - several small side fixes
> >
> > - example perf eBPF code in bpf-samples/bpf-stdout-example.c
> > to show the build ID support/usage.
> >
> > # perf record -vv -e ./bpf-samples/bpf-stdout-example.c kill 2>&1 | grep buildid
> > libbpf: section(7) buildid, size 21, link 0, flags 3, type=1
> > libbpf: kernel buildid of ./bpf-samples/bpf-stdout-example.c is: 6e25edeb408513184e2753bebad25d42314501a0
> >
> > The buildid is provided the same way we provide kernel
> > version, in a special "buildid" section:
> >
> > # cat ./bpf-samples/bpf-stdout-example.c
> > ...
> > #include <linux/buildid.h>
> >
> > char _buildid[] SEC("buildid") = LINUX_BUILDID_DATA;
> > ...
> >
> > where LINUX_BUILDID_DATA is defined in the generated buildid.h.
> >
> > please note it's an RFC ;-) any comments and suggestions are welcome
>
> I think this is overkill.
>
> We're very heavy users of kprobe+bpf. It's used for lots
> of different cases and usage is constantly growing,
> but I haven't seen a single case of :
>
> > The eBPF kprobe program thus then get loaded for different
> > kernel than it's been built for, get wrong data (silently)
> > and provide misleading output.
>
> but I saw plenty of the opposite. People pre-compile the program
> and hack kernel version when they load, since they know in advance
> that kprobe+bpf doesn't use any kernel specific things.
> The existing kernel version check for kprobe+bpf is already annoying
> to them.
perhaps verifier could detect this (via bpf_probe_read usage) and disable
the version check automaticaly for such program?
and in the same way force the version check (or buildid when enabled)
once the bpf_probe_read is detected
thanks,
jirka
^ permalink raw reply
* Re: [PATCH v2] net: thunderx: rework mac addresses list to u64 array
From: David Miller @ 2018-04-06 15:06 UTC (permalink / raw)
To: Vadim.Lomovtsev
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, dnelson, gustavo, Vadim.Lomovtsev
In-Reply-To: <20180406111425.14636-1-Vadim.Lomovtsev@caviumnetworks.com>
From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
Date: Fri, 6 Apr 2018 04:14:25 -0700
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
> index 5fc46c5a4f36..448d1fafc827 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -265,14 +265,9 @@ struct nicvf_drv_stats {
>
> struct cavium_ptp;
>
> -struct xcast_addr {
> - struct list_head list;
> - u64 addr;
> -};
> -
> struct xcast_addr_list {
> - struct list_head list;
> int count;
> + u64 mc[];
> };
>
> struct nicvf_work {
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index 1e9a31fef729..a26d8bc92e01 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> work.work);
> struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + u8 idx = 0;
^^^^^^^^^^^
>
> if (!vf_work)
> return;
> @@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> /* check if we have any specific MACs to be added to PF DMAC filter */
> if (vf_work->mc) {
> /* now go through kernel list of MACs and add them one by one */
> - list_for_each_entry_safe(xaddr, next,
> - &vf_work->mc->list, list) {
> + for (idx = 0; idx < vf_work->mc->count; idx++) {
vf_work->mx->count is an 'int' therefore 'idx' should be declared 'int' as well,
not a 'u8'.
^ permalink raw reply
* Re: TCP one-by-one acking - RFC interpretation question
From: Michal Kubecek @ 2018-04-06 15:03 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <65f7ac65-e6d1-11f2-408e-39eb542ec817@gmail.com>
On Fri, Apr 06, 2018 at 05:01:29AM -0700, Eric Dumazet wrote:
>
>
> On 04/06/2018 03:05 AM, Michal Kubecek wrote:
> > Hello,
> >
> > I encountered a strange behaviour of some (non-linux) TCP stack which
> > I believe is incorrect but support engineers from the company producing
> > it claim is OK.
> >
> > Assume a client (sender, Linux 4.4 kernel) sends a stream of MSS sized
> > segments but segments 2, 4 and 6 do not reach the server (receiver):
> >
> > ACK SAK SAK SAK
> > +-------+-------+-------+-------+-------+-------+-------+
> > | 1 | 2 | 3 | 4 | 5 | 6 | 7 |
> > +-------+-------+-------+-------+-------+-------+-------+
> > 34273 35701 37129 38557 39985 41413 42841 44269
> >
> > When segment 2 is retransmitted after RTO timeout, normal response would
> > be ACK-ing segment 3 (38557) with SACK for 5 and 7 (39985-41413 and
> > 42841-44269).
> >
> > However, this server stack responds with two separate ACKs:
> >
> > - ACK 37129, SACK 37129-38557 39985-41413 42841-44269
> > - ACK 38557, SACK 39985-41413 42841-44269
>
> Hmmm... Yes this seems very very wrong and lazy.
>
> Have you verified behavior of more recent linux kernel to such threats ?
No, unfortunately the problem was only encountered by our customer in
production environment (they tried to reproduce in a test lab but no
luck). They are running backups to NFS server and it happens from time
to time (in the order of hours, IIUC). So it would be probably hard to
let them try with more recent kernel.
On the other hand, they reported that SLE11 clients (kernel 3.0) do not
run into this kind of problem. It was originally reported as a
a regression on migration from SLE11-SP4 (3.0 kernel) to SLE12-SP2 (4.4
kernel) and the problem was reported as "SLE12-SP2 is ignoring dupacks"
(which seems to be mostly caused by the switch to RACK).
It also seems that part of the problem is specific packet loss pattern
where at some point, many packets are lost in "every second" pattern.
The customer finally started to investigate this problem and it seems it
has something to do with their bonding setup (they provided no details,
my guess is packets are divided over two paths and one of them fails).
> packetdrill test would be relatively easy to write.
I'll try but I have very little experience with writing packetdrill
scripts so it will probably take some time.
> Regardless of this broken alien stack, we might be able to work around
> this faster than the vendor is able to fix and deploy a new stack.
>
> ( https://en.wikipedia.org/wiki/Robustness_principle )
> Be conservative in what you do, be liberal in what you accept from
> others...
I was thinking about this a bit. "Fixing" the acknowledgment number
could do the trick but it doesn't feel correct. We might use the fact
that TSecr of both ACKs above matches TSval of the retransmission which
triggered them so that RTT calculated from timestamp would be the right
one. So perhaps something like "prefer timestamp RTT if measured RTT
seems way too off". But I'm not sure if it couldn't break other use
cases where (high) measured RTT is actually correct, rather than (low)
timestamp RTT.
Michal Kubecek
^ permalink raw reply
* Re: [RFC] ethtool: Support for driver private ioctl's
From: Jose Abreu @ 2018-04-06 14:51 UTC (permalink / raw)
To: Andrew Lunn, Jose Abreu
Cc: Florian Fainelli, David Miller, Jakub Jelinek, Jeff Garzik,
Tim Hockin, Eli Kupermann, Chris Leech, Scott Feldman,
Ben Hutchings, netdev, Joao Pinto
In-Reply-To: <20180406144701.GO17495@lunn.ch>
Hi Andrew,
On 06-04-2018 15:47, Andrew Lunn wrote:
> On Fri, Apr 06, 2018 at 02:51:15PM +0100, Jose Abreu wrote:
>> Hi Florian,
>>
>> On 05-04-2018 16:50, Florian Fainelli wrote:
>>> On 04/05/2018 03:47 AM, Jose Abreu wrote:
>>>> Hi All,
>>>>
>>>> I would like to know your opinion regarding adding support for
>>>> driver private ioctl's in ethtool.
>>>>
>>>> Background: Synopsys Ethernet IP's have a certain number of
>>>> features which can be reconfigured at runtime. Giving you two
>>>> examples: One of the most recent one is the safety features,
>>>> which can be enabled/disabled and forced at runtime.
> Hi Jose
>
> Is there a reason somebody would decide to use the Ethernet in
> 'unsafe' mode? Cannot you just turn it on by default?
Yes, its already on by default. I was just trying to give an
example of an user-reconfigurable feature, maybe it was not the
best one :)
Thanks and Best Regards,
Jose Miguel Abreu
>
> Andrew
^ permalink raw reply
* Re: [RFC] ethtool: Support for driver private ioctl's
From: Andrew Lunn @ 2018-04-06 14:47 UTC (permalink / raw)
To: Jose Abreu
Cc: Florian Fainelli, David Miller, Jakub Jelinek, Jeff Garzik,
Tim Hockin, Eli Kupermann, Chris Leech, Scott Feldman,
Ben Hutchings, netdev, Joao Pinto
In-Reply-To: <27c05ec7-1f85-8e4a-be06-70d6d80e8a10@synopsys.com>
On Fri, Apr 06, 2018 at 02:51:15PM +0100, Jose Abreu wrote:
> Hi Florian,
>
> On 05-04-2018 16:50, Florian Fainelli wrote:
> >
> > On 04/05/2018 03:47 AM, Jose Abreu wrote:
> >> Hi All,
> >>
> >> I would like to know your opinion regarding adding support for
> >> driver private ioctl's in ethtool.
> >>
> >> Background: Synopsys Ethernet IP's have a certain number of
> >> features which can be reconfigured at runtime. Giving you two
> >> examples: One of the most recent one is the safety features,
> >> which can be enabled/disabled and forced at runtime.
Hi Jose
Is there a reason somebody would decide to use the Ethernet in
'unsafe' mode? Cannot you just turn it on by default?
Andrew
^ permalink raw reply
* Re: [PATCH net-next] netns: filter uevents correctly
From: Eric W. Biederman @ 2018-04-06 14:45 UTC (permalink / raw)
To: Christian Brauner
Cc: Kirill Tkhai, davem, gregkh, netdev, linux-kernel, avagin, serge
In-Reply-To: <20180406130704.GB9263@gmail.com>
Christian Brauner <christian.brauner@canonical.com> writes:
> On Thu, Apr 05, 2018 at 10:59:49PM -0500, Eric W. Biederman wrote:
>> Christian Brauner <christian.brauner@canonical.com> writes:
>>
>> > On Thu, Apr 05, 2018 at 05:26:59PM +0300, Kirill Tkhai wrote:
>> >> On 05.04.2018 17:07, Christian Brauner wrote:
>> >> > On Thu, Apr 05, 2018 at 04:01:03PM +0300, Kirill Tkhai wrote:
>> >> >> On 04.04.2018 22:48, Christian Brauner wrote:
>> >> >>> commit 07e98962fa77 ("kobject: Send hotplug events in all network namespaces")
>> >> >>>
>> >> >>> enabled sending hotplug events into all network namespaces back in 2010.
>> >> >>> Over time the set of uevents that get sent into all network namespaces has
>> >> >>> shrunk. We have now reached the point where hotplug events for all devices
>> >> >>> that carry a namespace tag are filtered according to that namespace.
>> >> >>>
>> >> >>> Specifically, they are filtered whenever the namespace tag of the kobject
>> >> >>> does not match the namespace tag of the netlink socket. One example are
>> >> >>> network devices. Uevents for network devices only show up in the network
>> >> >>> namespaces these devices are moved to or created in.
>> >> >>>
>> >> >>> However, any uevent for a kobject that does not have a namespace tag
>> >> >>> associated with it will not be filtered and we will *try* to broadcast it
>> >> >>> into all network namespaces.
>> >> >>>
>> >> >>> The original patchset was written in 2010 before user namespaces were a
>> >> >>> thing. With the introduction of user namespaces sending out uevents became
>> >> >>> partially isolated as they were filtered by user namespaces:
>> >> >>>
>> >> >>> net/netlink/af_netlink.c:do_one_broadcast()
>> >> >>>
>> >> >>> if (!net_eq(sock_net(sk), p->net)) {
>> >> >>> if (!(nlk->flags & NETLINK_F_LISTEN_ALL_NSID))
>> >> >>> return;
>> >> >>>
>> >> >>> if (!peernet_has_id(sock_net(sk), p->net))
>> >> >>> return;
>> >> >>>
>> >> >>> if (!file_ns_capable(sk->sk_socket->file, p->net->user_ns,
>> >> >>> CAP_NET_BROADCAST))
>> >> >>> j return;
>> >> >>> }
>> >> >>>
>> >> >>> The file_ns_capable() check will check whether the caller had
>> >> >>> CAP_NET_BROADCAST at the time of opening the netlink socket in the user
>> >> >>> namespace of interest. This check is fine in general but seems insufficient
>> >> >>> to me when paired with uevents. The reason is that devices always belong to
>> >> >>> the initial user namespace so uevents for kobjects that do not carry a
>> >> >>> namespace tag should never be sent into another user namespace. This has
>> >> >>> been the intention all along. But there's one case where this breaks,
>> >> >>> namely if a new user namespace is created by root on the host and an
>> >> >>> identity mapping is established between root on the host and root in the
>> >> >>> new user namespace. Here's a reproducer:
>> >> >>>
>> >> >>> sudo unshare -U --map-root
>> >> >>> udevadm monitor -k
>> >> >>> # Now change to initial user namespace and e.g. do
>> >> >>> modprobe kvm
>> >> >>> # or
>> >> >>> rmmod kvm
>> >> >>>
>> >> >>> will allow the non-initial user namespace to retrieve all uevents from the
>> >> >>> host. This seems very anecdotal given that in the general case user
>> >> >>> namespaces do not see any uevents and also can't really do anything useful
>> >> >>> with them.
>> >> >>>
>> >> >>> Additionally, it is now possible to send uevents from userspace. As such we
>> >> >>> can let a sufficiently privileged (CAP_SYS_ADMIN in the owning user
>> >> >>> namespace of the network namespace of the netlink socket) userspace process
>> >> >>> make a decision what uevents should be sent.
>> >> >>>
>> >> >>> This makes me think that we should simply ensure that uevents for kobjects
>> >> >>> that do not carry a namespace tag are *always* filtered by user namespace
>> >> >>> in kobj_bcast_filter(). Specifically:
>> >> >>> - If the owning user namespace of the uevent socket is not init_user_ns the
>> >> >>> event will always be filtered.
>> >> >>> - If the network namespace the uevent socket belongs to was created in the
>> >> >>> initial user namespace but was opened from a non-initial user namespace
>> >> >>> the event will be filtered as well.
>> >> >>> Put another way, uevents for kobjects not carrying a namespace tag are now
>> >> >>> always only sent to the initial user namespace. The regression potential
>> >> >>> for this is near to non-existent since user namespaces can't really do
>> >> >>> anything with interesting devices.
>> >> >>>
>> >> >>> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
>> >> >>> ---
>> >> >>> lib/kobject_uevent.c | 10 +++++++++-
>> >> >>> 1 file changed, 9 insertions(+), 1 deletion(-)
>> >> >>>
>> >> >>> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
>> >> >>> index 15ea216a67ce..cb98cddb6e3b 100644
>> >> >>> --- a/lib/kobject_uevent.c
>> >> >>> +++ b/lib/kobject_uevent.c
>> >> >>> @@ -251,7 +251,15 @@ static int kobj_bcast_filter(struct sock *dsk, struct sk_buff *skb, void *data)
>> >> >>> return sock_ns != ns;
>> >> >>> }
>> >> >>>
>> >> >>> - return 0;
>> >> >>> + /*
>> >> >>> + * The kobject does not carry a namespace tag so filter by user
>> >> >>> + * namespace below.
>> >> >>> + */
>> >> >>> + if (sock_net(dsk)->user_ns != &init_user_ns)
>> >> >>> + return 1;
>> >> >>> +
>> >> >>> + /* Check if socket was opened from non-initial user namespace. */
>> >> >>> + return sk_user_ns(dsk) != &init_user_ns;
>> >> >>> }
>> >> >>> #endif
>> >> >>
>> >> >> So, this prohibits to listen events of all devices except network-related
>> >> >> in containers? If it's so, I don't think it's a good solution. Uevents is not
>> >> >
>> >> > No, this is not correct: As it is right now *without my patch* no
>> >> > non-initial user namespace is receiving *any uevents* but those
>> >> > specifically namespaced such as those for network devices. This patch
>> >> > doesn't change that at all. The commit message outlines this in detail
>> >> > how this comes about.
>> >> > There is only one case where this currently breaks and this is as I
>> >> > outlined explicitly in my commit message when you create a new user
>> >> > namespace and map container(0) -> host(0). This patch fixes this.
>> >>
>> >> Could you please point the place, where non-initial user namespaces are filtered?
>> >> I only see the kobj_bcast_filter() logic, and it used to return 0, which means "accepted".
>> >> Now it will return 1 sometimes.
>> >
>> > Oh sure, it's in the commit message though. The callchain is
>> > lib/kobject_uevent.c:kobject_uevent_net_broadcast() ->
>> > nnet/netlink/af_netlink.c:netlink_broadcast_filtered() ->
>> > net/netlink/af_netlink.c:do_one_broadcast():
>> >
>> > This codepiece will check whether the openened socket holds
>> > CAP_NET_BROADCAST in the user namespace of the target network namespace
>> > which it won't because we don't have device namespaces and all devices
>> > belong to the initial set of namespaces.
>> >
>> > if (!file_ns_capable(sk->sk_socket->file, p->net->user_ns,
>> > CAP_NET_BROADCAST))
>> > j return;
>> >
>>
>> The above that only applies if someone has set NETLINK_F_LISTEN_ALL_NSID
>> on their socket and has had someone with the appropriate privileges
>> assign a peerrnetid.
>>
>> All of which is to say that file_ns_capable is not nearly as applicable
>> as it might be, and if you can pass the other two checks I think it is
>> pointless (because the peernet attributes are not generated for
>> kobj_uevents) but valid to receive events from outside your network
>> namespace.
>>
>>
>> I might be missing something but I don't see anything excluding network
>> namespaces owned by !init_user_ns excluded from the kobject_uevent
>> logic.
>>
>> The uevent_sock_list has one entry per network namespace. And
>> kobject_uevent_net_broacast appears to walk each one.
>
> Yeah, it definitely does.
>
>>
>> I had a memory of filtering uevent messages and I had a memory
>> that the netlink_has_listeners had a per network namespace effect.
>> Neither seems true from my inspection of the code tonight.
>
> Yeah, I drew the same conclusion.
>
>>
>> If we are not filtering ordinary uevents at least at the user namespace
>> level that does seem to be at least a nuisance.
>
> This patch would filter based on user namespace and bump it from "we're
> accidently doing the right thing" to "we're doing the right on purpose"
> and without accidently leaking uevents in some corner cases.
> Using kobj_bcast_filter() for this seems like the right thing to do.
> This sounds like you don't disagree with the approach I'm taking here?
How are we accidentally filtering? If we can not describe how the code
currently works to achieve something we don't understand it well enough
to change it.
> On a sidenote, it also very much feels like we're leaking information if
> we're not filtering based on user namespaces on purpose. Non-initial
> user namespaces should not be able to receive information about device
> attribues simply via netlink uevent messages. At least not *trivially*
> [1] by opening a netlink uevent socket.
This is not at all about isolation. Devices don't belong to user
namespaces. This is about it being useless/silly to send events
to those sockets as the receiver could not do anything with the uevent.
At a practical level there should be no receivers. Plus performance
issues. At least my memory is that any unprivileged user on the system
is allowed to listen to those events.
At least at one point udev listening and reacting to events in every
network namespace in containers was a significant slow down on the
system.
The report was that adding netlink_has_listeners greatly sped up uevent
sending. That testing appears faulty. I think the real gain was
userspace not listening to uevents in every container.
Which means uevent injection for containers has some real technical
challenges to overcome before it can be wide spread.
>> Christian can you dig a little deeper into this. I have a feeling that
>> there are some real efficiency improvements that we could make to
>> kobject_uevent_net_broadcast if nothing else.
>
> Yeah, for sure! I already started doing this. This patch here is
> basically a preparatory patch for that work which is on my todo.
Limiting the network namespaces on uevent_sock_list to just network
namespaces owned by the initial user namespaces would be a lot better
than your patch.
At a practical level I am concerned what will happen when we stand up a
uevent listener aka udev in say 100 or maybe 1000 unprivileged
containers on a system. History suggests the current data structures
won't scale to the problem, and the fixes that were put in place kernel
side only did not change anything. It was only the userspace fixes
that made a difference.
Which suggests either improving the isolation between network namespaces
for netlink multicast sockets or putting:
if (!net_eq(sock_net(sk), p->net)) {
if (!(nlk->flags & NETLINK_F_LISTEN_ALL_NSID))
return;
if (!peernet_has_id(sock_net(sk), p->net))
return;
if (!file_ns_capable(sk->sk_socket->file, p->net->user_ns,
CAP_NET_BROADCAST))
return;
}
In a filter and having an appropriate filter so that
netlink_broadcast_filtered only needs to be called once from
kobject_uevent_net_broadcast.
It is more difficult but in practice I expect we have far more to gain
by fixing the mc_list and the netlink_has_listeners function to be per
network namespace basis. Handling the NETLINK_F_LISTEN_ALL_NSID will
be trickier in that case but the current semantics appear correct
for that case.
But before we do anything we have to test and see if the assertion
that we don't make it to netlink listeners in network namespaces
that are outside of the init_user_ns is true. If it is true we need to
find the code that makes it true.
Eric
^ permalink raw reply
* Re: [PATCH net 1/3] lan78xx: PHY DSP registers initialization to address EEE link drop issues with long cables
From: Andrew Lunn @ 2018-04-06 14:43 UTC (permalink / raw)
To: Raghuram Chary J; +Cc: davem, netdev, unglinuxdriver, woojung.huh
In-Reply-To: <20180406061204.18257-2-raghuramchary.jallipalli@microchip.com>
On Fri, Apr 06, 2018 at 11:42:02AM +0530, Raghuram Chary J wrote:
> The patch is to configure DSP registers of PHY device
> to handle Gbe-EEE failures with >40m cable length.
>
> Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
> Signed-off-by: Raghuram Chary J <raghuramchary.jallipalli@microchip.com>
> ---
> drivers/net/phy/microchip.c | 123 ++++++++++++++++++++++++++++++++++++++++++-
> include/linux/microchipphy.h | 8 +++
> 2 files changed, 130 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c
> index 0f293ef28935..174ae9808722 100644
> --- a/drivers/net/phy/microchip.c
> +++ b/drivers/net/phy/microchip.c
> @@ -20,6 +20,7 @@
> #include <linux/ethtool.h>
> #include <linux/phy.h>
> #include <linux/microchipphy.h>
> +#include <linux/delay.h>
>
> #define DRIVER_AUTHOR "WOOJUNG HUH <woojung.huh@microchip.com>"
> #define DRIVER_DESC "Microchip LAN88XX PHY driver"
> @@ -66,6 +67,107 @@ static int lan88xx_suspend(struct phy_device *phydev)
> return 0;
> }
>
> +static void lan88xx_TR_reg_set(struct phy_device *phydev, u16 regaddr,
> + u32 data)
> +{
> + int val;
> + u16 buf;
> +
> + /* Get access to token ring page */
> + phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
> + LAN88XX_EXT_PAGE_ACCESS_TR);
Hi Raghuram
You might want to look at phy_read_paged(), phy_write_paged(), etc.
There can be race conditions with paged access.
Andrew
^ permalink raw reply
* Re: [PATCH net 2/3] lan78xx: Add support to dump lan78xx registers
From: Andrew Lunn @ 2018-04-06 14:40 UTC (permalink / raw)
To: Raghuram Chary J; +Cc: davem, netdev, unglinuxdriver, woojung.huh
In-Reply-To: <20180406061204.18257-3-raghuramchary.jallipalli@microchip.com>
> +static int lan78xx_get_regs_len(struct net_device *netdev)
> +{
> + return (sizeof(lan78xx_regs) + PHY_REG_SIZE);
If there is no PHY attached, you probably should not include
PHY_REG_SIZE here.
Andrew
^ permalink raw reply
* Re: [PATCH net 3/3] lan78xx: Lan7801 Support for Fixed PHY
From: Andrew Lunn @ 2018-04-06 14:35 UTC (permalink / raw)
To: Raghuram Chary J; +Cc: davem, netdev, unglinuxdriver, woojung.huh
In-Reply-To: <20180406061204.18257-4-raghuramchary.jallipalli@microchip.com>
On Fri, Apr 06, 2018 at 11:42:04AM +0530, Raghuram Chary J wrote:
> Adding Fixed PHY support to the lan78xx driver.
Hi Raghuram
What do you expect is connected to the MAC if there is no PHY?
Andrew
^ permalink raw reply
* Re: Enable and configure storm prevention in a network device
From: Andrew Lunn @ 2018-04-06 14:30 UTC (permalink / raw)
To: Florian Fainelli; +Cc: David Miller, m-karicheri2, netdev
In-Reply-To: <75eb56a4-ded2-5ed5-116c-776312f93cf3@gmail.com>
On Thu, Apr 05, 2018 at 03:35:06PM -0700, Florian Fainelli wrote:
> On 04/05/2018 01:20 PM, David Miller wrote:
> > From: Murali Karicheri <m-karicheri2@ti.com>
> > Date: Thu, 5 Apr 2018 16:14:49 -0400
> >
> >> Is there a standard way to implement and configure storm prevention
> >> in a Linux network device?
> >
> > What kind of "storm", an interrupt storm?
> >
>
> I would assume Murali is referring to L2 broadcast storms which is
> common in switches. There is not an API for that AFAICT and I am not
> sure what a proper API would look like.
tc?
The Marvell switches have leaky buckets, which can be used for
limiting broadcast and multicast packets, as well as traffic shaping
in general. Storm prevention is just a form of traffic shaping, so if
we have generic traffic shaping, it can be used for storm prevention.
Andrew
^ permalink raw reply
* Re: [PATCH net] net: dsa: Discard frames from unused ports
From: Andrew Lunn @ 2018-04-06 14:25 UTC (permalink / raw)
To: Florian Fainelli; +Cc: David Miller, netdev, Vivien Didelot
In-Reply-To: <9421e733-ed1a-0e2e-b587-4f32dad6d8cb@gmail.com>
On Thu, Apr 05, 2018 at 03:20:14PM -0700, Florian Fainelli wrote:
> On 04/04/2018 07:17 PM, Andrew Lunn wrote:
> > On Wed, Apr 04, 2018 at 05:49:10PM -0700, Florian Fainelli wrote:
> >> On 04/04/2018 04:56 PM, Andrew Lunn wrote:
> >>> The Marvell switches under some conditions will pass a frame to the
> >>> host with the port being the CPU port. Such frames are invalid, and
> >>> should be dropped. Not dropping them can result in a crash when
> >>> incrementing the receive statistics for an invalid port.
> >>>
> >>> Reported-by: Chris Healy <cphealy@gmail.com>
> >>> Fixes: 5f6b4e14cada ("net: dsa: User per-cpu 64-bit statistics")
> >>
> >> Are you sure this is the commit that introduced the problem?
> >
> > Hi Florian
> >
> > Well, the problem is it crashes when trying to update the
> > statistics. The CPU port is not allocated a p->stats64, only slave
> > ports get those. So before this patch, there was no crash and the
> > frame would be delivered to the master interface. This in itself is
> > probably not correct, but also not fatal. Talking to Chris, it seems
> > this behaviour has existing for a long while. I needed to use lldpd to
> > trigger the issue, because i assume the Marvell switch sees these as
> > special frames and forwards them to the CPU. The other thing is, the
> > code got refactored recently. So this fix will not rebase to too many
> > earlier versions. It needs a fix per tagging protocol for before the
> > common dsa_master_find_slave() was added.
>
> Yes what you are explaining makes sense, but does not that mean we would
> just be accessing a garbage memory location before as well?
Humm, yes. I actually picked the wrong patch. It took two attempts to
get the stats64 working. I should of picked the first one.
Before stats64, we just used skb->dev. I need to look back at older
code, but skb->dev is valid in the versions i tested. It points to the
master device. So we don't crash.
However, i agree, we should fix this for the LTS kernels.
Andrew
^ 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