* Re: [net PATCH v2 1/1] drivers: net: cpsw: dual_emac: fix reducing of rx descriptor during ifdown
From: David Miller @ 2014-09-10 20:59 UTC (permalink / raw)
To: mugunthanvnm; +Cc: netdev
In-Reply-To: <1410347289-3555-1-git-send-email-mugunthanvnm@ti.com>
From: Mugunthan V N <mugunthanvnm@ti.com>
Date: Wed, 10 Sep 2014 16:38:09 +0530
> In Dual EMAC, when both interface are up and while doing ifdown with heavy
> traffic then skbs already processed by DMA from that slave emac has to be
> requeued as still the other interface is up and running.
>
> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] tipc: fix sparse warnings
From: David Miller @ 2014-09-10 21:01 UTC (permalink / raw)
To: erik.hugne; +Cc: netdev, tipc-discussion
In-Reply-To: <1410350570-6145-1-git-send-email-erik.hugne@ericsson.com>
From: <erik.hugne@ericsson.com>
Date: Wed, 10 Sep 2014 14:02:50 +0200
> From: Erik Hugne <erik.hugne@ericsson.com>
>
> This fixes the following sparse warnings:
> sparse: symbol 'tipc_update_nametbl' was not declared. Should it be static?
> Also, the function is changed to return bool upon success, rather than a
> potentially freed pointer.
>
> Signed-off-by: Erik Hugne <erik.hugne@ericsson.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] mac80211: Check correct skb for shared states before freeing original
From: Alexander Duyck @ 2014-09-10 21:01 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev, linux-wireless, davem, linville, Eric Dumazet
In-Reply-To: <1410381187.2761.10.camel@jlt4.sipsolutions.net>
On 09/10/2014 01:33 PM, Johannes Berg wrote:
> On Wed, 2014-09-10 at 16:06 -0400, Alexander Duyck wrote:
>> The code for cloning the skb for an acknowledgement was checking to see if
>> the cloned skb was shared and if it was it was then freeing the original
>> skb. Since a clone should never really be shared I suspect that the
>> intention was to avoid freeing the clone if the original was shared. As
>> such I am updating the code so that if the original is shared we free the
>> original and use the clone. This avoids unnecessary work in the next
>> section where we would be cloning the skb if the original is shared.
>
> Thanks, yeah, I admit that this is clearly fishy.
>
>> @@ -2087,7 +2087,7 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>> if (id >= 0) {
>> info_id = id;
>> info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
>
> Luckily, we practically always go into this path.
>
>> - } else if (skb_shared(skb)) {
>> + } else if (skb_shared(orig_skb)) {
>> kfree_skb(orig_skb);
>> } else {
>> kfree_skb(skb);
>
> We have a clone already so we could just remove the whole "else if" I
> think, but I'm guessing my intent was to keep it accounted to the socket
> where possible rather than freeing the original in all cases.
>
> So yeah, I think this makes sense. Maybe we should add a comment to the
> if though to explain this?
>
> johannes
Actually I think we may need to take a different approach. The reason I
was in this code was to take a look at a possible refcount issue.
I'll be submitting another patch in a few minutes and will probably be
dropping some of this code anyway.
Thanks,
Alex
^ permalink raw reply
* Re: [PATCH net-next 0/5] Allow FW size upto 1MB, support for S25FL032P flash and misc. fixes
From: David Miller @ 2014-09-10 21:02 UTC (permalink / raw)
To: hariprasad; +Cc: netdev, leedom, nirranjan, kumaras, anish
In-Reply-To: <1410351271-26428-1-git-send-email-hariprasad@chelsio.com>
From: Hariprasad Shenai <hariprasad@chelsio.com>
Date: Wed, 10 Sep 2014 17:44:26 +0530
> This patch series adds support to allow FW size upto 1MB, support for S25FL032P
> flash. Fix t4_flash_erase_sectors to throw an error, when erase sector aren't in
> the flash and also warning message when adapters have flashes less than 2Mb.
> Adds device id of new adapter and removes device id of debug adapter.
>
> The patches series is created against 'net-next' tree.
> And includes patches on cxgb4 driver and cxgb4vf driver.
>
> We have included all the maintainers of respective drivers. Kindly review the
> change and let us know in case of any review comments.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next v2] net: bpf: only build bpf_jit_binary_{alloc,free}() when jit selected
From: David Miller @ 2014-09-10 21:05 UTC (permalink / raw)
To: dborkman; +Cc: fengguang.wu, ast, netdev
In-Reply-To: <1410354062-25337-1-git-send-email-dborkman@redhat.com>
From: Daniel Borkmann <dborkman@redhat.com>
Date: Wed, 10 Sep 2014 15:01:02 +0200
> Since BPF JIT depends on the availability of module_alloc() and
> module_free() helpers (HAVE_BPF_JIT and MODULES), we better build
> that code only in case we have BPF_JIT in our config enabled, just
> like with other JIT code. Fixes builds for arm/marzen_defconfig
> and sh/rsk7269_defconfig.
>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Fixes: 738cbe72adc5 ("net: bpf: consolidate JIT binary allocator")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
> v1->v2:
> - Sorry, we can reduce it further and remove the else part of
> the code as bpf_jit_binary_{alloc,free} helpers are only used
> from within JIT code anyway, of course.
Applied, thanks.
^ permalink raw reply
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Luis R. Rodriguez @ 2014-09-10 21:10 UTC (permalink / raw)
To: Tom Gundersen
Cc: One Thousand Gnomes, Takashi Iwai, Kay Sievers, Sreekanth Reddy,
James Bottomley, Praveen Krishnamoorthy, hare,
Nagalakshmi Nandigama, Wu Zhangjin, Tetsuo Handa,
mpt-fusionlinux.pdl, Tim Gardner, Benjamin Poirier,
Santosh Rastapur, Casey Leedom, Hariprasad S, Pierre Fersing,
Arjan van de Ven, Abhijit Mahajan, systemd Mailing List
In-Reply-To: <CAG-2HqUiUksdT4UyeDsdc1Q74=2BiX2TuzzGh_hoLiO5_DOvXQ@mail.gmail.com>
Tom, thanks for reviewing this! My reply below!
On Tue, Sep 9, 2014 at 11:46 PM, Tom Gundersen <teg@jklm.no> wrote:
> On Tue, Sep 9, 2014 at 10:45 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
>> On Tue, Sep 9, 2014 at 12:35 PM, James Bottomley
>> <James.Bottomley@hansenpartnership.com> wrote:
>>> On Tue, 2014-09-09 at 12:16 -0700, Luis R. Rodriguez wrote:
>>>> On Mon, Sep 8, 2014 at 10:38 PM, James Bottomley
>>>> <James.Bottomley@hansenpartnership.com> wrote:
>>>> > If we want to sort out some sync/async mechanism for probing devices, as
>>>> > an agreement between the init systems and the kernel, that's fine, but
>>>> > its a to-be negotiated enhancement.
>>>>
>>>> Unfortunately as Tejun notes the train has left which already made
>>>> assumptions on this.
>>>
>>> Well, that's why it's a bug. It's a material regression impacting
>>> users.
>>
>> Indeed. I believe the issue with this regression however was that the
>> original commit e64fae55 (January 2012) was only accepted by *kernel
>> folks* to be a real regression until recently.
>
> Just for the record, this only caused user-visible problems after
> kernel commit 786235ee (November 2013), right?
Another one was cxgb4:
https://bugzilla.novell.com/show_bug.cgi?id=877622
SLE12 does not yet have commit 786235ee merged. Benjamim did some hard
work to debug this and trace the kill down to systemd-udev. A debug
kernel build has been provided now to try to pick up exactly on the
place where the kill was received, but its at least clear this came
from systemd.
>> More than two years
>> have gone by on growing design and assumptions on top of that original
>> commit. I'm not sure if *systemd folks* yet believe its was a design
>> regression?
>
> I don't think so. udev should not allow its workers to run for an
> unbounded length of time. Whether the upper bound should be 30, 60,
> 180 seconds or something else is up for debate (currently it is 60,
> but if that is too short for some drivers we could certainly revisit
> that).
That's the thing -- the timeout was put in place under the assumption
probe was asyncronous and its not, the driver core issues both module
init *and* probe together, the loader has to wait. That alone makes
the timeout a design flaw, and then systemd carried on top of that
design over two years after that. Its not systemd's fault, its just
that we never spoke about this as a design thing broadly and we should
have, and I will mention that even when the first issues creeped up,
the issue was still tossed back a driver problems. It was only until
recently that we realized that both init and probe run together that
we've been thinking about this problem differently. Systemd was trying
to ensure init on driver don't take long but its not init that is
taking long, its probe, and probe gets then penalized as the driver
core batches both init and probe synchronously before finishing module
loading. Furthermore as clarified by Tejun random userland is known to
exist that will wait indefinitely for module loading under the simple
assumption things *are done synchronously*, and its precisely why we
can't just blindly enable async probe upstream through a new driver
boolean as it can be unfair to this old userland. What is being
evaluated is to enable aync probe for *all* drivers through a new
general system-wide option. We cannot regress old userspace and
assumptions but we can create a new shiny universe.
> Moreover, it seems from this discussion that the aim is (still)
> that insmod should be near-instantaneous (i.e., not wait for probe),
The only reason that is being discussed is that systemd has not
accepted the timeout as a system design despite me pointing out the
original design flaw recently and at this point even if was accepted
as a design flaw it seems its too late. The approach taken to help
make all drivers async is simply an afterthought to give systemd what
it *thought* was in place, and it by no measure should be considered
the proper fix to the regression introduced by systemd, it may perhaps
be the right step long term for systemd systems given it goes with
what it assumed was there, but the timeout was flawed. Its not clear
if systemd can help with old kernels, it seems the ship has sailed and
there seems no options but for folks to work around that -- unless of
course some reasonable solution is found which doesn't break current
systemd design?
> so it seems to me that the basic design is correct and all we need is
> some temporary work-around and a way to better detect misbehaving
> drivers?
As part of this series I addressed hunting for the "misbehaving
drivers" in-kernel as I saw no progress on the systemd side of things
to non-fatally detect "misbehaving drivers" despite my original RFCs
and request for advice. I quote "misbehaving drivers" as its a flawed
view to consider them misbehaving now in light of clarifications of
how the driver core works in that it batches both init and probe
together always and we can't be penalizing long probes due to the fact
long probes are simply fine. My patch to pick up "misbehaving drivers"
drivers on the kernel front by picking up systemd's signal was
reactive but it was also simply braindead given the same exact reasons
why systemd's original timeout was flawed. We want a general solution
and we don't want to work around the root cause, in this case it was
systemd's assumption on how drivers work.
Keep in mind that the above just addresses kmod built-in cmd on
systemd, its where the timeout was introduced but as has been
clarified here assuming the same timeout on *all* other built-in
likely is likely pretty flawed as well and this does concern me. Its
why I mentioned that more than two years have gone by now on growing
design and assumptions on top of that original commit and its why its
hard for systemd to consider an alternative.
>>>> I'm afraid distributions that want to avoid this
>>>> sigkill at least on the kernel front will have to work around this
>>>> issue either on systemd by increasing the default timeout which is now
>>>> possible thanks to Hannes' changes or by some other means such as the
>>>> combination of a modified non-chatty version of this patch + a check
>>>> at the end of load_module() as mentioned earlier on these threads.
>>>
>>> Increasing the default timeout in systemd seems like the obvious bug fix
>>> to me. If the patch exists already, having distros that want it use it
>>> looks to be correct ... not every bug is a kernel bug, after all.
>>
>> Its merged upstream on systemd now, along with a few fixes on top of
>> it. I also see Kay merged a change to the default timeout to 60 second
>> on August 30. Its unclear if these discussions had any impact on that
>> decision or if that was just because udev firmware loading got now
>> ripped out. I'll note that the new 60 second timeout wouldn't suffice
>> for cxgb4 even if it didn't do firmware loading, its probe takes over
>> one full minute.
>>
>>> Negotiating a probe vs init split for drivers is fine too, but it's a
>>> longer term thing rather than a bug fix.
>>
>> Indeed. What I proposed with a multiplier for the timeout for the
>> different types of built in commands was deemed complex but saw no
>> alternatives proposed despite my interest to work on one and
>> clarifications noted that this was a design regression. Not quite sure
>> what else I could have done here. I'm interested in learning what the
>> better approach is for the future as if we want to marry init + kernel
>> we need a smooth way for us to discuss design without getting worked
>> up about it, or taking it personal. I really want this to work as I
>> personally like systemd so far.
>
> How about this: keep the timeout global, but also introduce a
> (relatively short, say 10 or 15 seconds) timeout after which a warning
> is printed.
That is something that I originally was looking forward to on systemd,
but here's the thing once that warning comes up -- what would we do
with it? This patch addresses this warning in-kernel and the idea was
that we'd then peg an async_probe bool as true on the driver as a fix,
that was decided to be silly given all the above. These drivers are
actually not misbehaving and it would be even more incorrect to try to
"fix" them by making them run asynchronously. In fact for some old
storage drivers it may even be the worst thing to do given the
possible slew of userland deployment and scripts which assume things
*are* synchronous.
> Even if nothing is actually killed, having workers (be it
> insmod or something else) take longer than a couple of seconds is
> likely a sign that something is seriously off somewhere.
Probe can take a long time and that's fine, so for kmod the current
assumption is flawed. If we had an option to async probe all drivers
then perhaps this kmod timeout *might be reasonable*, and even then I
do recommend for a clear warning that can be collected on logs on its
first iteration rather than sigkilling, only after a whlie should
sigkilling be done really. If systemd can deal with module loading in
the background for drivers that take a long time and warning on that
intsead of sigkiling it may be good start prior to enabling a default
sigkill on drivers. This is perhaps also true for other workers but
its not clear if this is a reasonable strategy for systemd.
Luis
^ permalink raw reply
* [PATCH v2 net] ipv6: clean up anycast when an interface is destroyed
From: Sabrina Dubroca @ 2014-09-10 21:23 UTC (permalink / raw)
To: David Miller; +Cc: cwang, netdev, hannes
In-Reply-To: <20140910.135840.133555874665251669.davem@davemloft.net>
If we try to rmmod the driver for an interface while sockets with
setsockopt(JOIN_ANYCAST) are alive, some refcounts aren't cleaned up
and we get stuck on:
unregister_netdevice: waiting for ens3 to become free. Usage count = 1
If we LEAVE_ANYCAST/close everything before rmmod'ing, there is no
problem.
We need to perform a cleanup similar to the one for multicast in
addrconf_ifdown(how == 1).
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
v2: remove comment
include/net/addrconf.h | 1 +
net/ipv6/addrconf.c | 8 +++++---
net/ipv6/anycast.c | 21 +++++++++++++++++++++
3 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index f679877bb601..ec51e673b4b6 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -204,6 +204,7 @@ void ipv6_sock_ac_close(struct sock *sk);
int ipv6_dev_ac_inc(struct net_device *dev, const struct in6_addr *addr);
int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr);
+void ipv6_ac_destroy_dev(struct inet6_dev *idev);
bool ipv6_chk_acast_addr(struct net *net, struct net_device *dev,
const struct in6_addr *addr);
bool ipv6_chk_acast_addr_src(struct net *net, struct net_device *dev,
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index fc1fac2a0528..3342ee64f2e3 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3094,11 +3094,13 @@ static int addrconf_ifdown(struct net_device *dev, int how)
write_unlock_bh(&idev->lock);
- /* Step 5: Discard multicast list */
- if (how)
+ /* Step 5: Discard anycast and multicast list */
+ if (how) {
+ ipv6_ac_destroy_dev(idev);
ipv6_mc_destroy_dev(idev);
- else
+ } else {
ipv6_mc_down(idev);
+ }
idev->tstamp = jiffies;
diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
index ff2de7d9d8e6..9a386842fd62 100644
--- a/net/ipv6/anycast.c
+++ b/net/ipv6/anycast.c
@@ -351,6 +351,27 @@ static int ipv6_dev_ac_dec(struct net_device *dev, const struct in6_addr *addr)
return __ipv6_dev_ac_dec(idev, addr);
}
+void ipv6_ac_destroy_dev(struct inet6_dev *idev)
+{
+ struct ifacaddr6 *aca;
+
+ write_lock_bh(&idev->lock);
+ while ((aca = idev->ac_list) != NULL) {
+ idev->ac_list = aca->aca_next;
+ write_unlock_bh(&idev->lock);
+
+ addrconf_leave_solict(idev, &aca->aca_addr);
+
+ dst_hold(&aca->aca_rt->dst);
+ ip6_del_rt(aca->aca_rt);
+
+ aca_put(aca);
+
+ write_lock_bh(&idev->lock);
+ }
+ write_unlock_bh(&idev->lock);
+}
+
/*
* check if the interface has this anycast address
* called with rcu_read_lock()
--
2.1.0
^ permalink raw reply related
* Re: [Patch net-next 3/5] ipv6: clean up ipv6_dev_ac_inc()
From: Cong Wang @ 2014-09-10 21:32 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Linux Kernel Network Developers, Hideaki YOSHIFUJI,
David S. Miller
In-Reply-To: <1410351815.3135.14.camel@localhost>
On Wed, Sep 10, 2014 at 5:23 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Di, 2014-09-09 at 16:52 -0700, Cong Wang wrote:
>> + in6_dev_hold(idev);
>
> Please move this in6_dev_hold down to where it gets attached to the
> ifacaddr6 and remove the in6_dev_put from below the out: label.
I thought it is for idev->lock in case of idev becomes a wild pointer
as well, but it seems its callers or at least RTNL should guarantee
idev is still valid. So yeah, then we can probably move it down.
>
>> write_lock_bh(&idev->lock);
>> if (idev->dead) {
>> err = -ENODEV;
>> @@ -267,7 +262,7 @@ int ipv6_dev_ac_inc(struct net_device *dev, const struct in6_addr *addr)
>> aca->aca_users = 1;
>> /* aca_tstamp should be updated upon changes */
>> aca->aca_cstamp = aca->aca_tstamp = jiffies;
>> - atomic_set(&aca->aca_refcnt, 2);
>> + atomic_set(&aca->aca_refcnt, 1);
>> spin_lock_init(&aca->aca_lock);
>>
>> aca->aca_next = idev->ac_list;
>> @@ -276,9 +271,7 @@ int ipv6_dev_ac_inc(struct net_device *dev, const struct in6_addr *addr)
>>
>> ip6_ins_rt(rt);
>>
>> - addrconf_join_solict(dev, &aca->aca_addr);
>> -
>> - aca_put(aca);
>
> I am not sure why you changed the aca_refcnt code. idev->ac_list is only
> protected by idev->lock and you publish one reference and unlock, thus
> you need a second reference during addrconf_join_solict. All accesses
> should also be protected by rtnl, so it shouldn't be a problem, but if
> people review the code they might have problems to figure that out.
> Maybe you can also remove the idev->lock?
>
I was misled by the code, it is not easy to understand this since there
is no comment. I will refactor the code to make it more readable and
of course in a separated patch.
Thanks.
^ permalink raw reply
* Re: Question about synchronize_net() in AF_PACKET close()
From: Martin Kelly @ 2014-09-10 21:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Paul McKenney, Stephen Hemminger
In-Reply-To: <20140909.150000.1647602729214485795.davem@davemloft.net>
> The synchronize_net() is also there to protect against the prot hook
> which can run asynchronously from the core packet input path on any
> cpu.
>
Yes, understood. What I'm not clear about is whether it is safe to do
the following:
unregister_prot_hook(sk, false);
sock_orphan(sk);
sock->sk = NULL;
call_rcu(...);
close socket, return to userspace
instead of
unregister_prot_hook(sk, false);
synchronize_net();
sock_orphan(sk);
sock->sk = NULL;
close socket, return to userspace
If you don't call synchronize_net() immediately, then other readers
could see the protocol hook in the protocol list and try to use it.
They could call into prot_hook.func. However, it appears that such
functions ( e.g. packet_rcv() ) touch the socket buffer but not the
socket itself, so orphaning the socket before all RCUs have been
processed is safe. In addition, no new packets will come in after
packet_release() and touch the socket because the socket fd will be
removed from the process fd list.
>From my testing, I'm not seeing any obvious issues, but I could be
missing something. Is orphaning the socket before all RCUs have
finished unsafe?
^ permalink raw reply
* Re: Question about synchronize_net() in AF_PACKET close()
From: Martin Kelly @ 2014-09-10 21:39 UTC (permalink / raw)
To: mleitner; +Cc: David Miller, netdev, Paul McKenney, Stephen Hemminger
In-Reply-To: <54104FE6.4030404@redhat.com>
>
>
> And also http://patchwork.ozlabs.org/patch/181605/
> Which was an attempt on improving it.
>
> Marcelo
>
Yes, the patch I'm testing right now is very similar to the one
mentioned in that link. Whether or not the optimization is worth the
added complexity is a separate discussion. For now, I'm interested in
whether the optimization can be safely performed, both for improving
close() performance and for my own understanding of the code.
^ permalink raw reply
* [PATCH net-next v2 1/2] skb: Add documentation for skb_clone_sk
From: Alexander Duyck @ 2014-09-10 22:05 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
Cc: johannes-cdvu00un1VgdHxzADdlk8Q, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
linville-2XuSBdqkA4R54TAoqtyWWQ
In-Reply-To: <20140910215837.23225.39149.stgit-+uVpp3jiz/QKn9AQLGuxw7vm/XP+8Wra@public.gmane.org>
This change adds some documentation to the call skb_clone_sk. This is
meant to help clarify the purpose of the function for other developers.
Signed-off-by: Alexander Duyck <alexander.h.duyck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
v2: Updated comments to specifically call out need for sock_hold/sock_put
net/core/skbuff.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index a18dfb0..c9da77a 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3511,6 +3511,19 @@ struct sk_buff *sock_dequeue_err_skb(struct sock *sk)
}
EXPORT_SYMBOL(sock_dequeue_err_skb);
+/**
+ * skb_clone_sk - create clone of skb, and take reference to socket
+ * @skb: the skb to clone
+ *
+ * This function creates a clone of a buffer that holds a reference on
+ * sk_refcnt. Buffers created via this function are meant to be
+ * returned using sock_queue_err_skb, or free via kfree_skb.
+ *
+ * When passing buffers allocated with this function to sock_queue_err_skb
+ * it is necessary to wrap the call with sock_hold/sock_put in order to
+ * prevent the socket from being released prior to being enqueued on
+ * the sk_error_queue.
+ */
struct sk_buff *skb_clone_sk(struct sk_buff *skb)
{
struct sock *sk = skb->sk;
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* RFC: DSA device/driver model revamp
From: Florian Fainelli @ 2014-09-10 22:11 UTC (permalink / raw)
To: netdev, davem, jogo, nbd, alexander.duyck, jhs, kernel
Hi all,
This email is a DSA re-architecting proposal aiming at making it more
useful for non-MDIO managed switch drivers.
Problem statement:
DSA was mostly designed around MDIO-managed switches, and as such took a
lot of shortcuts in how these devices are discovered.
Whether we are using plain C structures or Device Tree, this all boils
down to instantiating a "dsa" platform_device that contains information
on how the switch ports are available, where the switch is sitting on
the attached Ethernet controller MDIO bus, and consequently which
Ethernet controller and MDIO bus to use.
For each DSA switch driver registered, the DSA platform device code will
call into the DSA switch driver with the probe() callback and pass it a
struct mii_bus, ultimately asking the switch driver to read the switch
device ID from the MDIO bus and telling it go/no-go with what it read.
This basically forces non-MDIO drivers to return something that just
makes the code happy, but does not match the truth.
Consequently, we never have a 'struct dsa_switch' backed by an actual
'struct device', which prevents proper Power Management, and other
goodies such as the device-managed helper functions from being used, it
also somehow violates the Device/driver layering.
The very fact that all the DSA registration is driven by the "dsa"
platform device also makes it hard to dynamically insert switch devices
and have something modular, since everything must be known at the time
this driver is registered.
Proposed solution:
Instead of evolving around this DSA platform_device/driver, we want
something that comes from the host interface driver. Whether that driver
is a PHY, SPI, I2C client, platform or PCI driver does not matter, what
matters is that we are backed by an actual struct device, and we follow
the Linux Device Driver model.
Moving forward, what we would be seeing is something like this:
- a given driver (PCI, SPI, PHY) gets probed
- allocates a struct dsa_switch
- assigns all relevant members:
- dsa_switch_driver becomes dsa_switch_driver_ops
- assigns a parent struct device pointer
- declares the type of tagging protocol supported by this device
- assigns the correct device_node pointers/platform configuration
pointers to communicate this down to the DSA core code
- registers the dsa_switch by calling dsa_switch_register()
very much like what a regular network device driver does.
Open questions:
There is some legacy code that will instantiate the special "dsa"
platform device in arch/arm/mach-orion5x/*, this code needs to be
transitioned one way or the other.
The switches used by these Orion5x platforms are MDIO-connected
switches, with the proposal above, these switch drivers will become PHY
drivers matching a particular PHY ID on the MDIO bus.
We currently do not have a way to attach platform specific data that is
not coming from Device Tree to these PHY devices, something potentially
not too intrusive could be to extend phy_register_fixup() with a void *
argument to pass specific platform data, or create
phy_register_fixup_with_data().
Comments welcome!
--
Florian
^ permalink raw reply
* [PATCH net-next 0/2] Address reference counting issues with sock_queue_err_skb
From: Alexander Duyck @ 2014-09-10 22:04 UTC (permalink / raw)
To: netdev, linux-wireless; +Cc: johannes, davem, eric.dumazet, linville
After looking over the code for skb_clone_sk after some comments made by
Eric Dumazet I have come to the conclusion that skb_clone_sk is taking the
correct approach in how to handle the sk_refcnt when creating a buffer that
is eventually meant to be returned to the socket via the sock_queue_err_skb
function.
However upon review of other callers I found what I believe to be a
possible reference count issue in the path for handling "wifi ack" packets.
To address this I have applied the same logic that is currently in place so
that the sk_refcnt will be forced to stay at least 1, or we will not
provide an skb to return in the sk_error_queue.
---
Alexander Duyck (2):
skb: Add documentation for skb_clone_sk
mac80211: Resolve sk_refcnt/sk_wmem_alloc issue in wifi ack path
net/core/skbuff.c | 18 ++++++++++++++++++
net/mac80211/tx.c | 15 ++++-----------
2 files changed, 22 insertions(+), 11 deletions(-)
--
^ permalink raw reply
* [PATCH net-next 2/2] mac80211: Resolve sk_refcnt/sk_wmem_alloc issue in wifi ack path
From: Alexander Duyck @ 2014-09-10 22:05 UTC (permalink / raw)
To: netdev, linux-wireless; +Cc: johannes, davem, eric.dumazet, linville
In-Reply-To: <20140910215837.23225.39149.stgit@ahduyck-bv4.jf.intel.com>
There is a possible issue with the use, or lack thereof of sk_refcnt and
sk_wmem_alloc in the wifi ack status functionality.
Specifically if a socket were to request acknowledgements, and the socket
were to have sk_refcnt drop to 0 resulting in it waiting on sk_wmem_alloc
to reach 0 it would be possible to have sock_queue_err_skb orphan the last
buffer, resulting in __sk_free being called on the socket. After this the
buffer is enqueued on sk_error_queue, however the queue has already been
flushed resulting in at least a memory leak, if not a data corruption.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
net/core/skbuff.c | 5 +++++
net/mac80211/tx.c | 15 ++++-----------
2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index c9da77a..c8259ac 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3628,9 +3628,14 @@ void skb_complete_wifi_ack(struct sk_buff *skb, bool acked)
serr->ee.ee_errno = ENOMSG;
serr->ee.ee_origin = SO_EE_ORIGIN_TXSTATUS;
+ /* take a reference to prevent skb_orphan() from freeing the socket */
+ sock_hold(sk);
+
err = sock_queue_err_skb(sk, skb);
if (err)
kfree_skb(skb);
+
+ sock_put(sk);
}
EXPORT_SYMBOL_GPL(skb_complete_wifi_ack);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 925c39f..cf71414 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2072,30 +2072,23 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
if (unlikely(!multicast && skb->sk &&
skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS)) {
- struct sk_buff *orig_skb = skb;
+ struct sk_buff *ack_skb = skb_clone_sk(skb);
- skb = skb_clone(skb, GFP_ATOMIC);
- if (skb) {
+ if (ack_skb) {
unsigned long flags;
int id;
spin_lock_irqsave(&local->ack_status_lock, flags);
- id = idr_alloc(&local->ack_status_frames, orig_skb,
+ id = idr_alloc(&local->ack_status_frames, ack_skb,
1, 0x10000, GFP_ATOMIC);
spin_unlock_irqrestore(&local->ack_status_lock, flags);
if (id >= 0) {
info_id = id;
info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
- } else if (skb_shared(skb)) {
- kfree_skb(orig_skb);
} else {
- kfree_skb(skb);
- skb = orig_skb;
+ kfree_skb(ack_skb);
}
- } else {
- /* couldn't clone -- lose tx status ... */
- skb = orig_skb;
}
}
^ permalink raw reply related
* Re: [PATCH net 0/4] mlx4 SRIOV fixes
From: David Miller @ 2014-09-10 22:23 UTC (permalink / raw)
To: ogerlitz; +Cc: netdev, jackm, amirv
In-Reply-To: <1410356516-23594-1-git-send-email-ogerlitz@mellanox.com>
From: Or Gerlitz <ogerlitz@mellanox.com>
Date: Wed, 10 Sep 2014 16:41:52 +0300
> This series contains few SRIOV related fixes from Matan, please apply to net.
Series applied.
^ permalink raw reply
* Re: [PATCH net] mlx4: Fix wrong endianess access with QP context flags
From: David Miller @ 2014-09-10 22:23 UTC (permalink / raw)
To: ogerlitz; +Cc: netdev, matanb, amirv
In-Reply-To: <1410358511-13461-1-git-send-email-ogerlitz@mellanox.com>
From: Or Gerlitz <ogerlitz@mellanox.com>
Date: Wed, 10 Sep 2014 17:15:11 +0300
> We wrongly tested QP context bits without BE conversion
> as was spotted by sparse...
>
> drivers/infiniband/hw/mlx4/qp.c:1685:38: sparse: restricted __be32 degrades to integer
>
> Fix that!
>
> Fixes: d2fce8a ('mlx4: Set user-space raw Ethernet QPs to properly handle VXLAN traffic')
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH] Documentation: filter: Add MIPS to architectures with BPF JIT
From: David Miller @ 2014-09-10 22:24 UTC (permalink / raw)
To: markos.chandras; +Cc: netdev, rdunlap, dborkman, ast, linux-doc, linux-kernel
In-Reply-To: <1410357104-30490-1-git-send-email-markos.chandras@imgtec.com>
From: Markos Chandras <markos.chandras@imgtec.com>
Date: Wed, 10 Sep 2014 14:51:44 +0100
> MIPS supports BPF JIT since v3.16-rc1
>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Daniel Borkmann <dborkman@redhat.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Acked-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH net-next v2 0/2] sunvnet: Reduce LDC message overhead.
From: David Miller @ 2014-09-10 22:25 UTC (permalink / raw)
To: sowmini.varadhan; +Cc: raghuram.kothakota, netdev
In-Reply-To: <20140910140434.GA14466@oracle.com>
From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Wed, 10 Sep 2014 10:04:34 -0400
> Excuse me if I am missing something obvious, but what was the
> verdict on this? Is there any additional clarification/information
> I can provide, if this is being mulled over?
After the memory barrier stuff has been sorted out, the patches
should be submitted again for review.
But I was under the impression that for now we weren't going to
be doing the new udelay() things, which defers patch #2 for now.
^ permalink raw reply
* Re: [Patch net-next 4/5] ipv6: drop ipv6_sk_mc_lock in mcast
From: Cong Wang @ 2014-09-10 22:36 UTC (permalink / raw)
To: Sabrina Dubroca
Cc: Linux Kernel Network Developers, Hannes Frederic Sowa,
Hideaki YOSHIFUJI, David S. Miller
In-Reply-To: <20140910171623.GA12299@kria>
On Wed, Sep 10, 2014 at 10:16 AM, Sabrina Dubroca <sd@queasysnail.net> wrote:
>
> Just a small thing: there are a few comments in the file that still
> refer to this lock (as ipv6_sk_mc_lock or ip6_sk_mc_lock):
>
Yeah, I missed it. But it is already out of date since RCU conversion.
I will fix the comment in a separated patch in the next update.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next v2 0/2] sunvnet: Reduce LDC message overhead.
From: Sowmini Varadhan @ 2014-09-10 22:37 UTC (permalink / raw)
To: David Miller; +Cc: raghuram.kothakota, netdev
In-Reply-To: <20140910.152528.391406680350942661.davem@davemloft.net>
On (09/10/14 15:25), David Miller wrote:
>
> After the memory barrier stuff has been sorted out, the patches
> should be submitted again for review.
>
> But I was under the impression that for now we weren't going to
> be doing the new udelay() things, which defers patch #2 for now.
ah, ok, I didn't realize the ball was in my court- good that I checked.
I'll re-submit a unified diff without the extra-udelay-stuff
as a v3 patch for review tomorrow.
--Sowmini
^ permalink raw reply
* Re: [PATCH] pktgen: nowait parameter.
From: Rusty Russell @ 2014-09-10 23:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, brouer, minipli
In-Reply-To: <20140905.142618.2240630803375505977.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
> From: Rusty Russell <rusty@rustcorp.com.au>
> Date: Wed, 03 Sep 2014 13:50:01 +0930
>
>> While trying to measure speed of virtio_net, I was getting hangs.
>> This is because we skb_orphan() but delay the tx interrupt
>> indefinitely (by number of slots).
>>
>> With nowait, pktgen won't wait for the skb to be released. This
>> introduces an error, but it's ok if count >> ringsize.
>>
>> I updated the documentation, but it needs far more work (it
>> refers to pgset and an examples directory, none of which exist
>> in the kernel tree).
>>
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Please just make this a flag, like UDPCSUM, NO_TIMESTAMP, et al.
> Which also means that it should be capitalized.
Agreed, though I prefer Jason's IFF_TX_SKB_FREE_DELAY, which doesn't
require intimate knowledge of the driver to get the option correct.
> BTW, wrt. holding onto TX frames for unbounded amounts of time, I
> think this is a bad idea even with skb_orphan(). There are resources
> from the SKB you are hanging onto which can stall the removal of
> modules indefinitely, such as netfilter references.
We could certainly have a once-a-second timer which did this, but should
skb_orphan() do that work instead?
Cheers,
Rusty.
^ permalink raw reply
* Re: [Patch net-next 0/5] ipv6: clean up locking in anycast and mcast
From: Cong Wang @ 2014-09-10 23:54 UTC (permalink / raw)
To: David Miller
Cc: Linux Kernel Network Developers, Hannes Frederic Sowa,
Hideaki YOSHIFUJI
In-Reply-To: <20140910.130150.1203685634918422464.davem@davemloft.net>
On Wed, Sep 10, 2014 at 1:01 PM, David Miller <davem@davemloft.net> wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Tue, 9 Sep 2014 16:52:13 -0700
>
>> This patchset cleans up the locking in anycast.c and mcast.c.
>>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>
> Please update this based upon the feedback you've received.
>
Sure thing, done.
^ permalink raw reply
* Re: [PATCH net-next 00/20] Support v6 real servers in v4 pools and vice versa
From: Simon Horman @ 2014-09-10 23:55 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Julian Anastasov, Alex Gartrell, netdev, lvs-devel, kernel-team,
ps, David Miller
In-Reply-To: <20140910151617.GA4830@salvia>
On Wed, Sep 10, 2014 at 05:16:17PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 10, 2014 at 03:22:01PM +0900, Simon Horman wrote:
> > [ CC Pablo, Dave M ]
> >
> > On Wed, Sep 10, 2014 at 09:01:34AM +0300, Julian Anastasov wrote:
> > >
> > > Hello,
> > >
> > > On Tue, 9 Sep 2014, Alex Gartrell wrote:
> > >
> > > > The following patch set has been iterated upon in the lvs-devel mailing
> > > > list. I am submitting it here per Julian's advice for inclusion in the
> > > > net-next tree.
> > >
> > > Acked-by: Julian Anastasov <ja@ssi.bg>
> > >
> > > Simon, please apply the patchset to ipvs-next.
> > > I guess ipvs-next and nf-next need to be updated
> > > before that due to dependency on net-next commit ea1d5d7755a3e556
> > > ("ipvs: properly declare tunnel encapsulation").
> >
> > Thanks Julian, yes that does seem to be the case.
> >
> > Pablo, could you merge nf into nf-next and push the result
> > so that I can apply this series on top?
>
> Let me flush out my nf-next queue first, many updates in it, I'll ping
> to let you know once you can prepare your series. Thanks.
Thanks, much appreciated.
^ permalink raw reply
* Re: [PATCH ipvs v2] ipvs: Add simple weighted failover scheduler
From: Simon Horman @ 2014-09-10 23:56 UTC (permalink / raw)
To: Kenny Mathis; +Cc: Julian Anastasov, netdev, Wensong Zhang
In-Reply-To: <CAPvRj6herHa82QA9iszdT1+AMu05UJML427W4wct14NZ=suYZA@mail.gmail.com>
On Wed, Sep 10, 2014 at 09:18:24AM -0400, Kenny Mathis wrote:
> From what I can tell, ipvsadm doesn't need any specific code updates. The
> only place the schedulers are actually listed is in the man page and the
> SCHEDULERS file, so I could submit a change to ensure it's listed. When I
> tested with keepalived, the failover (fo) module showed up properly under
> ipvsadm as well without any modifications.
Thanks, I understand.
I think updating SCHEDULERS would be good but clearly its not urgent.
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] bonding: Simplify the xmit function for modes that use xmit_hash
From: Mahesh Bandewar @ 2014-09-11 0:12 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David Miller,
netdev, Eric Dumazet, Maciej Zenczykowski
In-Reply-To: <54105025.9040802@redhat.com>
On Wed, Sep 10, 2014 at 6:20 AM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:
> On 10/09/14 02:46, Mahesh Bandewar wrote:
>>
>> Earlier change to use usable slave array for TLB mode had an additional
>> performance advantage. So extending the same logic to all other modes
>> that use xmit-hash for slave selection (viz 802.3AD, and XOR modes).
>> Also consolidating this with the earlier TLB change.
>>
>> The main idea is to build the usable slaves array in the control path
>> and use that array for slave selection during xmit operation.
>>
>> Measured performance in a setup with a bond of 4x1G NICs with 200
>> instances of netperf for the modes involved (3ad, xor, tlb)
>> cmd: netperf -t TCP_RR -H <TargetHost> -l 60 -s 5
>>
>> Mode TPS-Before TPS-After
>>
>> 802.3ad : 468,694 493,101
>> TLB (lb=0): 392,583 392,965
>> XOR : 475,696 484,517
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>> ---
>> v1:
>> (a) If bond_update_slave_arr() fails to allocate memory, it will
>> overwrite
>> the slave that need to be removed.
>> (b) Freeing of array will assign NULL (to handle bond->down to bond->up
>> transition gracefully.
>> (c) Change from pr_debug() to pr_err() if bond_update_slave_arr()
>> returns
>> failure.
>> (d) XOR: bond_update_slave_arr() will consider mii-mon, arp-mon cases
>> and
>> will populate the array even if these parameters are not used.
>> (e) 3AD: Should handle the ad_agg_selection_logic correctly.
>> v2:
>> (a) Removed rcu_read_{un}lock() calls from array manipulation code.
>> (b) Slave link-events now refresh array for all these modes.
>> (c) Moved free-array call from bond_close() to bond_uninit().
>>
>> drivers/net/bonding/bond_3ad.c | 76 +++----------------
>> drivers/net/bonding/bond_alb.c | 51 ++-----------
>> drivers/net/bonding/bond_alb.h | 8 --
>> drivers/net/bonding/bond_main.c | 158
>> +++++++++++++++++++++++++++++++++++++---
>> drivers/net/bonding/bonding.h | 8 ++
>> 5 files changed, 170 insertions(+), 131 deletions(-)
>>
> Hello Mahesh,
> First a question, if a bond device in XOR mode is up and we enslave a single
> slave how would it start transmitting ? Same question, if we are enslaving a
> second device then the array will be rebuild with only the first upon
> NETDEV_UP (of course all this is in the case miimon is 0).
> The NETDEV_UP upon enslave happens before the slave is linked in.
We definitely need to handle the NETDEV_UP event. If we do not then
the slave array stays stale when device is already enslaved and link
flaps. Now in case of enslaving the first and the second slave (the
case that you have mentioned), I do not see that behavior! If link
event happens before enslaving then the slave-array will not get
updated since the notifier call-chain (a) may not call bonding
notifier (b) even if it calls; the update_slave_arr() will miss it
since it will not be present in slaves list. I do not see that
happening.
OTOH bond_set_carrier() call is almost at the end of enslave() (which
is after slave is linked in) that might be triggering the NETDEV_UP
event which results in the array update.
> One side note, you can now simplify these patches and drop the checks for
> bond->lock since Dave applied my patches.
>
Yes, done.
> A few more important problems below.
>
>
>> diff --git a/drivers/net/bonding/bond_3ad.c
>> b/drivers/net/bonding/bond_3ad.c
>> index ee2c73a9de39..1549e718074a 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -1579,6 +1579,8 @@ static void ad_agg_selection_logic(struct aggregator
>> *agg)
>> __disable_port(port);
>> }
>> }
>> + if (bond_update_slave_arr(bond, NULL))
>> + pr_err("Failed to build slave-array for 3ad
>> mode.\n");
>> }
>>
>> /* if the selected aggregator is of join individuals
>> @@ -1717,6 +1719,8 @@ static void ad_enable_collecting_distributing(struct
>> port *port)
>> port->actor_port_number,
>> port->aggregator->aggregator_identifier);
>> __enable_port(port);
>> + if (bond_update_slave_arr(port->slave->bond, NULL))
>> + pr_err("Failed to build slave-array for 3ad
>> mode.\n");
>> }
>> }
>>
>> @@ -1733,6 +1737,8 @@ static void
>> ad_disable_collecting_distributing(struct port *port)
>> port->actor_port_number,
>> port->aggregator->aggregator_identifier);
>> __disable_port(port);
>> + if (bond_update_slave_arr(port->slave->bond, NULL))
>> + pr_err("Failed to build slave-array for 3ad
>> mode.\n");
>> }
>> }
>>
>> @@ -2311,6 +2317,9 @@ void bond_3ad_handle_link_change(struct slave
>> *slave, char link)
>> */
>> port->sm_vars |= AD_PORT_BEGIN;
>>
>> + if (bond_update_slave_arr(slave->bond, NULL))
>> + pr_err("Failed to build slave-array for 3ad mode.\n");
>> +
>> __release_state_machine_lock(port);
>> }
>>
>> @@ -2407,73 +2416,6 @@ int bond_3ad_get_active_agg_info(struct bonding
>> *bond, struct ad_info *ad_info)
>> return ret;
>> }
>>
>> -int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>> -{
>> - struct bonding *bond = netdev_priv(dev);
>> - struct slave *slave, *first_ok_slave;
>> - struct aggregator *agg;
>> - struct ad_info ad_info;
>> - struct list_head *iter;
>> - int slaves_in_agg;
>> - int slave_agg_no;
>> - int agg_id;
>> -
>> - if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>> - netdev_dbg(dev, "__bond_3ad_get_active_agg_info
>> failed\n");
>> - goto err_free;
>> - }
>> -
>> - slaves_in_agg = ad_info.ports;
>> - agg_id = ad_info.aggregator_id;
>> -
>> - if (slaves_in_agg == 0) {
>> - netdev_dbg(dev, "active aggregator is empty\n");
>> - goto err_free;
>> - }
>> -
>> - slave_agg_no = bond_xmit_hash(bond, skb) % slaves_in_agg;
>> - first_ok_slave = NULL;
>> -
>> - bond_for_each_slave_rcu(bond, slave, iter) {
>> - agg = SLAVE_AD_INFO(slave)->port.aggregator;
>> - if (!agg || agg->aggregator_identifier != agg_id)
>> - continue;
>> -
>> - if (slave_agg_no >= 0) {
>> - if (!first_ok_slave && bond_slave_can_tx(slave))
>> - first_ok_slave = slave;
>> - slave_agg_no--;
>> - continue;
>> - }
>> -
>> - if (bond_slave_can_tx(slave)) {
>> - bond_dev_queue_xmit(bond, skb, slave->dev);
>> - goto out;
>> - }
>> - }
>> -
>> - if (slave_agg_no >= 0) {
>> - netdev_err(dev, "Couldn't find a slave to tx on for
>> aggregator ID %d\n",
>> - agg_id);
>> - goto err_free;
>> - }
>> -
>> - /* we couldn't find any suitable slave after the agg_no, so use
>> the
>> - * first suitable found, if found.
>> - */
>> - if (first_ok_slave)
>> - bond_dev_queue_xmit(bond, skb, first_ok_slave->dev);
>> - else
>> - goto err_free;
>> -
>> -out:
>> - return NETDEV_TX_OK;
>> -err_free:
>> - /* no suitable interface, frame not sent */
>> - dev_kfree_skb_any(skb);
>> - goto out;
>> -}
>> -
>> int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding
>> *bond,
>> struct slave *slave)
>> {
>> diff --git a/drivers/net/bonding/bond_alb.c
>> b/drivers/net/bonding/bond_alb.c
>> index 73c21e233131..c44cabd8f2ba 100644
>> --- a/drivers/net/bonding/bond_alb.c
>> +++ b/drivers/net/bonding/bond_alb.c
>> @@ -200,7 +200,6 @@ static int tlb_initialize(struct bonding *bond)
>> static void tlb_deinitialize(struct bonding *bond)
>> {
>> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> - struct tlb_up_slave *arr;
>>
>> _lock_tx_hashtbl_bh(bond);
>>
>> @@ -208,10 +207,6 @@ static void tlb_deinitialize(struct bonding *bond)
>> bond_info->tx_hashtbl = NULL;
>>
>> _unlock_tx_hashtbl_bh(bond);
>> -
>> - arr = rtnl_dereference(bond_info->slave_arr);
>> - if (arr)
>> - kfree_rcu(arr, rcu);
>> }
>>
>> static long long compute_gap(struct slave *slave)
>> @@ -1409,39 +1404,9 @@ out:
>> return NETDEV_TX_OK;
>> }
>>
>> -static int bond_tlb_update_slave_arr(struct bonding *bond,
>> - struct slave *skipslave)
>> -{
>> - struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> - struct slave *tx_slave;
>> - struct list_head *iter;
>> - struct tlb_up_slave *new_arr, *old_arr;
>> -
>> - new_arr = kzalloc(offsetof(struct tlb_up_slave,
>> arr[bond->slave_cnt]),
>> - GFP_ATOMIC);
>> - if (!new_arr)
>> - return -ENOMEM;
>> -
>> - bond_for_each_slave(bond, tx_slave, iter) {
>> - if (!bond_slave_can_tx(tx_slave))
>> - continue;
>> - if (skipslave == tx_slave)
>> - continue;
>> - new_arr->arr[new_arr->count++] = tx_slave;
>> - }
>> -
>> - old_arr = rtnl_dereference(bond_info->slave_arr);
>> - rcu_assign_pointer(bond_info->slave_arr, new_arr);
>> - if (old_arr)
>> - kfree_rcu(old_arr, rcu);
>> -
>> - return 0;
>> -}
>> -
>> int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>> {
>> struct bonding *bond = netdev_priv(bond_dev);
>> - struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> struct ethhdr *eth_data;
>> struct slave *tx_slave = NULL;
>> u32 hash_index;
>> @@ -1462,12 +1427,14 @@ int bond_tlb_xmit(struct sk_buff *skb, struct
>> net_device *bond_dev)
>> hash_index &
>> 0xFF,
>> skb->len);
>> } else {
>> - struct tlb_up_slave *slaves;
>> + struct bond_up_slave *slaves;
>> + unsigned int count;
>>
>> - slaves =
>> rcu_dereference(bond_info->slave_arr);
>> - if (slaves && slaves->count)
>> + slaves = rcu_dereference(bond->slave_arr);
>> + count = slaves->count;
>
> ^^^^^^^^^^^^^^^
> What happens if slaves is still NULL ? We'll dereference a NULL pointer
> here.
> I'm beginning to think you don't test these submissions at all, I hit the
> obvious NULL pointer dereference immediately after trying to enslave a
> device in XOR mode with these patches applied.
>
I do test the code before sending it :)
I was using the following method to see the robustness of the change -
while true; do
echo -eth$(($RANDOM % 4 + 1)) >
/sys/class/net/eth0/bonding/slaves
sleep 1
echo +eth$(($RANDOM % 4 + 1)) >
/sys/class/net/eth0/bonding/slaves
sleep 1
done
while having some background traffic through the bond interface (BTW I
have a setup of 4 nics). It caught several issues but one things that
I missed was to start with no slaves and then add them one by one. My
setup always came up with 4 slaves and above script would change the
slave count randomly, but some how it never took it to zero. It looks
silly but this error could have been found if looked carefully, OTOH
this is functionally a big change and making changes to the collection
of hacks and coming out right first time is little difficult. So
please bare with me. On the positive note - because of you I learnt so
much about the XOR mode now! I definitely appreciate your inputs.
>
>> + if (slaves && count)
>> tx_slave = slaves->arr[hash_index
>> %
>> -
>> slaves->count];
>> + count];
>> }
>> break;
>> }
>> @@ -1733,10 +1700,6 @@ void bond_alb_deinit_slave(struct bonding *bond,
>> struct slave *slave)
>> rlb_clear_slave(bond, slave);
>> }
>>
>> - if (bond_is_nondyn_tlb(bond))
>> - if (bond_tlb_update_slave_arr(bond, slave))
>> - pr_err("Failed to build slave-array for TLB
>> mode.\n");
>> -
>> }
>>
>> /* Caller must hold bond lock for read */
>> @@ -1762,7 +1725,7 @@ void bond_alb_handle_link_change(struct bonding
>> *bond, struct slave *slave, char
>> }
>>
>> if (bond_is_nondyn_tlb(bond)) {
>> - if (bond_tlb_update_slave_arr(bond, NULL))
>> + if (bond_update_slave_arr(bond, NULL))
>> pr_err("Failed to build slave-array for TLB
>> mode.\n");
>> }
>> }
>> diff --git a/drivers/net/bonding/bond_alb.h
>> b/drivers/net/bonding/bond_alb.h
>> index aaeac61d03cf..5fc76c01636c 100644
>> --- a/drivers/net/bonding/bond_alb.h
>> +++ b/drivers/net/bonding/bond_alb.h
>> @@ -139,20 +139,12 @@ struct tlb_slave_info {
>> */
>> };
>>
>> -struct tlb_up_slave {
>> - unsigned int count;
>> - struct rcu_head rcu;
>> - struct slave *arr[0];
>> -};
>> -
>> struct alb_bond_info {
>> struct tlb_client_info *tx_hashtbl; /* Dynamically allocated */
>> spinlock_t tx_hashtbl_lock;
>> u32 unbalanced_load;
>> int tx_rebalance_counter;
>> int lp_counter;
>> - /* -------- non-dynamic tlb mode only ---------*/
>> - struct tlb_up_slave __rcu *slave_arr; /* Up slaves */
>> /* -------- rlb parameters -------- */
>> int rlb_enabled;
>> struct rlb_client_info *rx_hashtbl; /* Receive hash table */
>> diff --git a/drivers/net/bonding/bond_main.c
>> b/drivers/net/bonding/bond_main.c
>> index f0f5eab0fab1..456cda67383b 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1693,6 +1693,10 @@ static int __bond_release_one(struct net_device
>> *bond_dev,
>> if (BOND_MODE(bond) == BOND_MODE_8023AD)
>> bond_3ad_unbind_slave(slave);
>>
>> + if (bond_mode_uses_xmit_hash(bond) &&
>> + bond_update_slave_arr(bond, slave))
>> + pr_err("Failed to build slave-array.\n");
>> +
>> write_unlock_bh(&bond->lock);
>>
>> netdev_info(bond_dev, "Releasing %s interface %s\n",
>> @@ -2009,6 +2013,10 @@ static void bond_miimon_commit(struct bonding
>> *bond)
>> bond_alb_handle_link_change(bond, slave,
>> BOND_LINK_UP);
>>
>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>> + bond_update_slave_arr(bond, NULL))
>> + pr_err("Failed to build slave-array for
>> XOR mode.\n");
>> +
>> if (!bond->curr_active_slave ||
>> (slave == bond->primary_slave))
>> goto do_failover;
>> @@ -2037,6 +2045,10 @@ static void bond_miimon_commit(struct bonding
>> *bond)
>> bond_alb_handle_link_change(bond, slave,
>>
>> BOND_LINK_DOWN);
>>
>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>> + bond_update_slave_arr(bond, NULL))
>> + pr_err("Failed to build slave-array for
>> XOR mode.\n");
>> +
>> if (slave ==
>> rcu_access_pointer(bond->curr_active_slave))
>> goto do_failover;
>>
>> @@ -2500,6 +2512,9 @@ static void bond_loadbalance_arp_mon(struct
>> work_struct *work)
>>
>> if (slave_state_changed) {
>> bond_slave_state_change(bond);
>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>> + bond_update_slave_arr(bond, NULL))
>> + pr_err("Failed to build slave-array for
>> XOR mode.\n");
>> } else if (do_failover) {
>> /* the bond_select_active_slave must hold RTNL
>> * and curr_slave_lock for write.
>> @@ -2893,11 +2908,23 @@ static int bond_slave_netdev_event(unsigned long
>> event,
>> if (old_duplex != slave->duplex)
>> bond_3ad_adapter_duplex_changed(slave);
>> }
>> + /* Refresh slave-array if applicable!
>> + * If the setuo does not use miimon or arpmon
>> (mode-specific!),
>> + * then these event will not cause the slave-array to be
>> + * refreshed. This will cause xmit to use a slave that is
>> not
>> + * usable. Avoid such situation by refeshing the array at
>> these
>> + * events. If these (miimon/arpmon) parameters are
>> configured
>> + * then array gets refreshed twice and that should be
>> fine!
>> + */
>> + if (bond_mode_uses_xmit_hash(bond) &&
>> + bond_update_slave_arr(bond, NULL))
>> + pr_err("Failed to build slave-array for XOR
>> mode.\n");
>> break;
>> case NETDEV_DOWN:
>> - /*
>> - * ... Or is it this?
>> - */
>> + /* Refresh slave-array if applicable! */
>> + if (bond_mode_uses_xmit_hash(bond) &&
>> + bond_update_slave_arr(bond, NULL))
>> + pr_err("Failed to build slave-array for XOR
>> mode.\n");
>> break;
>> case NETDEV_CHANGEMTU:
>> /*
>> @@ -3143,6 +3170,10 @@ static int bond_open(struct net_device *bond_dev)
>> bond_3ad_initiate_agg_selection(bond, 1);
>> }
>>
>> + if (bond_mode_uses_xmit_hash(bond) &&
>> + bond_update_slave_arr(bond, NULL))
>> + pr_err("Failed to build slave-array for XOR mode.\n");
>> +
>> return 0;
>> }
>>
>> @@ -3684,15 +3715,112 @@ static int bond_xmit_activebackup(struct sk_buff
>> *skb, struct net_device *bond_d
>> return NETDEV_TX_OK;
>> }
>>
>> -/* In bond_xmit_xor() , we determine the output device by using a pre-
>> - * determined xmit_hash_policy(), If the selected device is not enabled,
>> - * find the next active slave.
>> +/* Build the usable slaves array in control path for modes that use
>> xmit-hash
>> + * to determine the slave interface -
>> + * (a) BOND_MODE_8023AD
>> + * (b) BOND_MODE_XOR
>> + * (c) BOND_MODE_TLB && tlb_dynamic_lb == 0
>> */
>> -static int bond_xmit_xor(struct sk_buff *skb, struct net_device
>> *bond_dev)
>> +int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>> {
>> - struct bonding *bond = netdev_priv(bond_dev);
>> + struct slave *slave;
>> + struct list_head *iter;
>> + struct bond_up_slave *new_arr, *old_arr;
>> + int slaves_in_agg;
>> + int agg_id = 0;
>> + int ret = 0;
>> +
>> + if (!bond_has_slaves(bond))
>> + goto out;
>> +
>> + new_arr = kzalloc(offsetof(struct bond_up_slave,
>> arr[bond->slave_cnt]),
>> + GFP_ATOMIC);
>> + if (!new_arr) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>> + struct ad_info ad_info;
>> +
>> + if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
>> + pr_debug("bond_3ad_get_active_agg_info failed\n");
>> + kfree_rcu(new_arr, rcu);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + slaves_in_agg = ad_info.ports;
>> + agg_id = ad_info.aggregator_id;
>> + }
>> + bond_for_each_slave(bond, slave, iter) {
>> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>> + struct aggregator *agg;
>> +
>> + agg = SLAVE_AD_INFO(slave)->port.aggregator;
>> + if (!agg || agg->aggregator_identifier != agg_id)
>> + continue;
>> + }
>> + if (!bond_slave_can_tx(slave))
>> + continue;
>> + if (skipslave == slave)
>> + continue;
>> + new_arr->arr[new_arr->count++] = slave;
>> + }
>>
>> - bond_xmit_slave_id(bond, skb, bond_xmit_hash(bond, skb) %
>> bond->slave_cnt);
>> + old_arr = rcu_dereference_protected(bond->slave_arr,
>> + lockdep_rtnl_is_held() ||
>> + lockdep_is_held(&bond->lock)
>> ||
>> +
>> lockdep_is_held(&bond->curr_slave_lock));
>> + rcu_assign_pointer(bond->slave_arr, new_arr);
>> + if (old_arr)
>> + kfree_rcu(old_arr, rcu);
>> +
>> +out:
>> + if (ret != 0 && skipslave) {
>> + int idx;
>> +
>> + /* Rare situation where caller has asked to skip a
>> specific
>> + * slave but allocation failed (most likely!). BTW this is
>> + * only possible when the call is initiated from
>> + * __bond_release_one(). In this sitation; overwrite the
>> + * skipslave entry in the array with the last entry from
>> the
>> + * array to avoid a situation where the xmit path may
>> choose
>> + * this to-be-skipped slave to send a packet out.
>> + */
>> + old_arr = rcu_dereference_protected(bond->slave_arr,
>> + lockdep_is_held(&bond->lock)
>> &&
>> + lockdep_rtnl_is_held());
>> + for (idx = 0; idx < old_arr->count; idx++) {
>> + if (skipslave == old_arr->arr[idx]) {
>> + old_arr->arr[idx] =
>> + old_arr->arr[old_arr->count-1];
>> + old_arr->count--;
>> + break;
>> + }
>> + }
>> + }
>> + return ret;
>> +}
>> +
>> +/* Use this Xmit function for 3AD as well as XOR modes. The current
>> + * usable slave array is formed in the control path. The xmit function
>> + * just calculates hash and sends the packet out.
>> + */
>> +int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
>> + struct bonding *bond = netdev_priv(dev);
>> + struct slave *slave;
>> + struct bond_up_slave *slaves;
>> + unsigned int count;
>> +
>> + slaves = rcu_dereference(bond->slave_arr);
>> + count = slaves->count;
>
> ^^^^^^^^^^^
> Same here, if slaves is NULL we'll dereference a NULL ptr.
>
>
>> + if (slaves && count) {
>> + slave = slaves->arr[bond_xmit_hash(bond, skb) % count];
>> + bond_dev_queue_xmit(bond, skb, slave->dev);
>> + } else {
>> + dev_kfree_skb_any(skb);
>> + atomic_long_inc(&dev->tx_dropped);
>> + }
>>
>> return NETDEV_TX_OK;
>> }
>> @@ -3794,12 +3922,11 @@ static netdev_tx_t __bond_start_xmit(struct
>> sk_buff *skb, struct net_device *dev
>> return bond_xmit_roundrobin(skb, dev);
>> case BOND_MODE_ACTIVEBACKUP:
>> return bond_xmit_activebackup(skb, dev);
>> + case BOND_MODE_8023AD:
>> case BOND_MODE_XOR:
>> - return bond_xmit_xor(skb, dev);
>> + return bond_3ad_xor_xmit(skb, dev);
>> case BOND_MODE_BROADCAST:
>> return bond_xmit_broadcast(skb, dev);
>> - case BOND_MODE_8023AD:
>> - return bond_3ad_xmit_xor(skb, dev);
>> case BOND_MODE_ALB:
>> return bond_alb_xmit(skb, dev);
>> case BOND_MODE_TLB:
>> @@ -3980,6 +4107,7 @@ static void bond_uninit(struct net_device *bond_dev)
>> struct bonding *bond = netdev_priv(bond_dev);
>> struct list_head *iter;
>> struct slave *slave;
>> + struct bond_up_slave *arr;
>>
>> bond_netpoll_cleanup(bond_dev);
>>
>> @@ -3988,6 +4116,12 @@ static void bond_uninit(struct net_device
>> *bond_dev)
>> __bond_release_one(bond_dev, slave->dev, true);
>> netdev_info(bond_dev, "Released all slaves\n");
>>
>> + arr = rtnl_dereference(bond->slave_arr);
>> + if (arr) {
>> + kfree_rcu(arr, rcu);
>> + RCU_INIT_POINTER(bond->slave_arr, NULL);
>> + }
>> +
>> list_del(&bond->bond_list);
>>
>> bond_debug_unregister(bond);
>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>> index 64b36ba65b89..5642cccd4469 100644
>> --- a/drivers/net/bonding/bonding.h
>> +++ b/drivers/net/bonding/bonding.h
>> @@ -177,6 +177,12 @@ struct slave {
>> struct kobject kobj;
>> };
>>
>> +struct bond_up_slave {
>> + unsigned int count;
>> + struct rcu_head rcu;
>> + struct slave *arr[0];
>> +};
>> +
>> /*
>> * Link pseudo-state only used internally by monitors
>> */
>> @@ -196,6 +202,7 @@ struct bonding {
>> struct slave __rcu *curr_active_slave;
>> struct slave __rcu *current_arp_slave;
>> struct slave *primary_slave;
>> + struct bond_up_slave __rcu *slave_arr; /* Array of usable slaves
>> */
>> bool force_primary;
>> s32 slave_cnt; /* never change this value outside the
>> attach/detach wrappers */
>> int (*recv_probe)(const struct sk_buff *, struct bonding *,
>> @@ -534,6 +541,7 @@ const char *bond_slave_link_status(s8 link);
>> struct bond_vlan_tag *bond_verify_device_path(struct net_device
>> *start_dev,
>> struct net_device *end_dev,
>> int level);
>> +int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
>>
>> #ifdef CONFIG_PROC_FS
>> void bond_create_proc_entry(struct bonding *bond);
>>
>
^ 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