* Re: [PATCH v3] ip: fix truesize mismatch in ip fragmentation
From: David Miller @ 2010-09-21 19:24 UTC (permalink / raw)
To: hmh; +Cc: eric.dumazet, nbowler, linux-kernel, netdev, jarkao2, kaber
In-Reply-To: <20100921180940.GA8754@khazad-dum.debian.net>
From: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Date: Tue, 21 Sep 2010 15:09:40 -0300
> On Tue, 21 Sep 2010, Eric Dumazet wrote:
>> Le mardi 21 septembre 2010 à 13:26 -0300, Henrique de Moraes Holschuh a
>> écrit :
>> > Should this be a candidate for -stable?
>> >
>>
>> Yes, of course, but David wants to handle stable submissions himself.
>>
>> I am not sure we want to bug stable team with dozens of mails while
>> polishing patches ?
>
> We don't. But one often marks commits that should go to -stable using a Cc:
> pseudo-header, and also includes relevant information (e.g. to which stable
> kernels it should be applied to) to the commit message.
>
> Since there wasn't one, and I didn't readly find any post in this thread
> that mentioned it should also go to stable, AND it looked at first glance
> like something that should go to stable, I asked about it.
Sorry, that is not how we typically handle things in the networking.
I queue up all appropriate -stable patches automatically.
I do this mainly because:
1) It isn't the submitter who gets to decide all by himself that
something is -stable material, that's partly my job too.
So if the submitter puts the CC: stable thing in the commit
message, that takes me out of the decision making process.
2) I do not want -stable submissions to go in just because a patch
made it into Linus's tree.
I want fixes to sit and cook in Linus's tree for a while before
they go to -stable unless it's an _incredibly_ obvious fix.
This allows any bugs in the fix to be shaken out first.
Again, the CC: stable tag subverts that.
I really think the "CC: stable" tag is only appropriate for a very
limited scope of bug fixes. The incredibly obvious ones that need
almost no testing and time exposure in Linus's tree.
All of the rest should be carefully queued up for -stable and
submitted there after a week or two of the patch sitting in Linus's
tree.
^ permalink raw reply
* Re: pull request: wireless-2.6 2010-09-21
From: David Miller @ 2010-09-21 19:26 UTC (permalink / raw)
To: linville-2XuSBdqkA4R54TAoqtyWWQ
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20100921182620.GB2448-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
From: "John W. Linville" <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Date: Tue, 21 Sep 2010 14:26:20 -0400
> Dave,
>
> Here are two more small fixes intended for 2.6.36. One is a one-liner
> (kmalloc->kzalloc) that prevents a potential information leak.
> The other is a small fix that skips firmware reloads (a workaround
> for firmware problems) while a scan is in progress in order to avoid
> the stack trace cited in the commit log.
>
> Please let me know if there are problems!
Pulled, thanks a lot John.
--
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
* Re: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2
From: David Miller @ 2010-09-21 19:33 UTC (permalink / raw)
To: amit.salecha; +Cc: eric.dumazet, netdev, ameen.rahman, anirban.chakraborty
In-Reply-To: <99737F4847ED0A48AECC9F4A1974A4B80F86F80278@MNEXMB2.qlogic.org>
From: Amit Salecha <amit.salecha@qlogic.com>
Date: Tue, 21 Sep 2010 03:41:42 -0500
>> So in order to improve "throughput", you were allowing for memory
>> exhaust and freeze of the _machine_ ?
>>
> This won't lead to such problem. truesize is used for accounting only.
Yes, it will.
Do you understand that we enforce both socket-level and system-wide
networking buffer usage in the stack? And this limiting is based
upon skb->truesize and therefore only works if skb->truesize is
accurate?
It's meant to keep people from attacking a server and consuming large
percentages of system memory with networking buffer memory such that
other tasks cannot complete successfully.
And by mis-reporting the truesize you are subverting that entirely.
This qlcnic truesize bug is a huge security hole, can't you see this
now?
^ permalink raw reply
* [PATCH] netxen: dont set skb->truesize
From: Eric Dumazet @ 2010-09-21 19:55 UTC (permalink / raw)
To: David Miller; +Cc: amit.salecha, netdev, ameen.rahman, anirban.chakraborty
In-Reply-To: <20100921.123338.71122967.davem@davemloft.net>
Le mardi 21 septembre 2010 à 12:33 -0700, David Miller a écrit :
> From: Amit Salecha <amit.salecha@qlogic.com>
> Date: Tue, 21 Sep 2010 03:41:42 -0500
>
> >> So in order to improve "throughput", you were allowing for memory
> >> exhaust and freeze of the _machine_ ?
> >>
> > This won't lead to such problem. truesize is used for accounting only.
>
> Yes, it will.
>
> Do you understand that we enforce both socket-level and system-wide
> networking buffer usage in the stack? And this limiting is based
> upon skb->truesize and therefore only works if skb->truesize is
> accurate?
>
> It's meant to keep people from attacking a server and consuming large
> percentages of system memory with networking buffer memory such that
> other tasks cannot complete successfully.
>
> And by mis-reporting the truesize you are subverting that entirely.
>
> This qlcnic truesize bug is a huge security hole, can't you see this
> now?
>
BTW, drivers/net/netxen/netxen_nic_init.c has same problem.
[PATCH] netxen: dont set skb->truesize
skb->truesize is set in core network.
Dont change it unless dealing with fragments.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
drivers/net/netxen/netxen_nic_init.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/netxen/netxen_nic_init.c b/drivers/net/netxen/netxen_nic_init.c
index cabae7b..b075a35 100644
--- a/drivers/net/netxen/netxen_nic_init.c
+++ b/drivers/net/netxen/netxen_nic_init.c
@@ -1540,7 +1540,6 @@ netxen_process_rcv(struct netxen_adapter *adapter,
if (pkt_offset)
skb_pull(skb, pkt_offset);
- skb->truesize = skb->len + sizeof(struct sk_buff);
skb->protocol = eth_type_trans(skb, netdev);
napi_gro_receive(&sds_ring->napi, skb);
@@ -1602,8 +1601,6 @@ netxen_process_lro(struct netxen_adapter *adapter,
skb_put(skb, lro_length + data_offset);
- skb->truesize = skb->len + sizeof(struct sk_buff) + skb_headroom(skb);
-
skb_pull(skb, l2_hdr_offset);
skb->protocol = eth_type_trans(skb, netdev);
^ permalink raw reply related
* Re: [PATCH] netfilter: xt_condition: add security capability support
From: Luciano Coelho @ 2010-09-21 19:56 UTC (permalink / raw)
To: ext Patrick McHardy
Cc: ext Jan Engelhardt, Makela Juhani.3 (EXT-Nixu/Helsinki),
ext Changli Gao, netfilter-devel@vger.kernel.org,
netdev@vger.kernel.org, James Morris,
linux-security-module@vger.kernel.org
In-Reply-To: <4C91292F.3090602@trash.net>
On Wed, 2010-09-15 at 22:14 +0200, ext Patrick McHardy wrote:
> Am 27.08.2010 09:55, schrieb Luciano Coelho:
> > That's what I tried to say when I said that we have a security team
> > taking care of this. They are implementing solutions to make the
> > product more secure, defending it against malware, misuse, attacks and
> > other such things. In this specific case, security-wise, we are trying
> > to prevent some bogus or malicious application from changing our
> > netfilter rules and causing havoc.
> >
> > LSM doesn't seem to be an option, here I quote Juhani (my colleague from
> > our security team):
> >
> >> The problem with capabilites is that they are assigned to binaries, not
> >> users. Kind of a setuid-mechanism, really. In our embedded environment
> >> that makes a lot of sense, but in a server-type environment with
> >> multiple users and a competent sysadmin, not so much. In such an
> >> environment using a LSM might also actually make sense. But for us it's
> >> not an option, mostly because LSMs are not stackable - you can have only
> >> one effective at any time - and I'm afraid we have already reserved some
> >> of the LSM hooks.
> >
> > Maybe Juhani can clarify this a bit more.
> >
> > One other idea that Juhani had was to add an option to the condition
> > match/target where the capability requiremets could be set, instead of
> > checking them by default. If nothing is specified, everything still
> > works as before (no caps checks). Or even a Kconfig option?
>
> I agree with Jan, adding module parameters to control permission checks
> or capabilities seems like a bad precedent.
Okay, the idea is now officially dropped. We'll use normal ACL to
control access to the conditions.
Thanks for your comments and sorry for trying to push ;)
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH RESEND 1/3] netfilter: xtables: inclusion of xt_condition
From: Luciano Coelho @ 2010-09-21 19:59 UTC (permalink / raw)
To: ext Patrick McHardy
Cc: netfilter-devel@vger.kernel.org, netdev@vger.kernel.org,
jengelh@medozas.de, sameo@linux.intel.com,
Ylalehto Janne (Nokia-MS/Tampere)
In-Reply-To: <4C912EFC.9070406@trash.net>
On Wed, 2010-09-15 at 22:39 +0200, ext Patrick McHardy wrote:
> Am 17.08.2010 10:36, schrieb Luciano Coelho:
> > diff --git a/net/netfilter/xt_condition.c b/net/netfilter/xt_condition.c
> > new file mode 100644
> > index 0000000..a78d832
> > --- /dev/null
> > +++ b/net/netfilter/xt_condition.c
> > @@ -0,0 +1,265 @@
> > +/*
> > + * "condition" match extension for Xtables
> > + *
> > + * Description: This module allows firewall rules to match using
> > + * condition variables available through procfs.
> > + *
> > + * Authors:
> > + * Stephane Ouellette <ouellettes [at] videotron ca>, 2002-10-22
> > + * Massimiliano Hofer <max [at] nucleus it>, 2006-05-15
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License; either version 2
> > + * or 3 of the License, as published by the Free Software Foundation.
> > + */
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +#include <linux/kernel.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/string.h>
> > +#include <linux/version.h>
> > +#include <linux/netfilter/x_tables.h>
> > +#include <linux/netfilter/xt_condition.h>
> > +#include <net/netns/generic.h>
> > +#include <asm/uaccess.h>
> > +
> > +/* Defaults, these can be overridden on the module command-line. */
> > +static unsigned int condition_list_perms = S_IRUGO | S_IWUSR;
> > +static unsigned int condition_uid_perms = 0;
> > +static unsigned int condition_gid_perms = 0;
>
> I'm not sure whether we already discussed this, but this isn't
> useful if namespaces are used since the IDs aren't global.
As Jan suggested, I think it would be better to include it like this,
since other modules already do similar things, and then I'll later send
a new patch with support for multiple namespaces.
> > +
> > +MODULE_AUTHOR("Stephane Ouellette <ouellettes@videotron.ca>");
> > +MODULE_AUTHOR("Massimiliano Hofer <max@nucleus.it>");
> > +MODULE_AUTHOR("Jan Engelhardt <jengelh@medozas.de>");
> > +MODULE_DESCRIPTION("Allows rules to match against condition variables");
> > +MODULE_LICENSE("GPL");
> > +module_param(condition_list_perms, uint, S_IRUSR | S_IWUSR);
> > +MODULE_PARM_DESC(condition_list_perms, "default permissions on /proc/net/nf_condition/* files");
> > +module_param(condition_uid_perms, uint, S_IRUSR | S_IWUSR);
> > +MODULE_PARM_DESC(condition_uid_perms, "default user owner of /proc/net/nf_condition/* files");
> > +module_param(condition_gid_perms, uint, S_IRUSR | S_IWUSR);
> > +MODULE_PARM_DESC(condition_gid_perms, "default group owner of /proc/net/nf_condition/* files");
> > +MODULE_ALIAS("ipt_condition");
> > +MODULE_ALIAS("ip6t_condition");
> > +
>
> > +/* proc_lock is a user context only semaphore used for write access */
> > +/* to the conditions' list. */
> > +static DEFINE_MUTEX(proc_lock);
>
> Why is this called proc_lock, as the comment states it protects
> the condition variable list? The comment is also misleading since
> this is a mutex and not a semaphore. I'd suggest list_mutex or
> condition_mutex.
I'll fix this.
> > +static int condition_proc_read(char __user *buffer, char **start, off_t offset,
> > + int length, int *eof, void *data)
> > +{
> > + const struct condition_variable *var = data;
> > +
> > + buffer[0] = var->enabled ? '1' : '0';
> > + buffer[1] = '\n';
>
> This is a user buffer, you need copy_to_user(). Also please run
> sparse against your code to check for similar errors.
Oooff, I actually also noticed this when integrating into our internal
tree. I'll fix this and run sparse properly before resending.
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH RESEND 2/3] netfilter: xt_condition: change the value from boolean to u32
From: Luciano Coelho @ 2010-09-21 20:00 UTC (permalink / raw)
To: ext Jan Engelhardt
Cc: Patrick McHardy, netfilter-devel@vger.kernel.org,
netdev@vger.kernel.org, sameo@linux.intel.com,
Ylalehto Janne (Nokia-MS/Tampere)
In-Reply-To: <alpine.LNX.2.01.1009152246180.5146@obet.zrqbmnf.qr>
On Wed, 2010-09-15 at 22:46 +0200, ext Jan Engelhardt wrote:
> On Wednesday 2010-09-15 22:41, Patrick McHardy wrote:
>
> >Am 17.08.2010 10:36, schrieb Luciano Coelho:
> >> static struct xt_match condition_mt_reg __read_mostly = {
> >> .name = "condition",
> >> - .revision = 1,
> >> + .revision = 2,
> >
> >Is this previous version actually compatible with anything that has
> >ever been released?
>
> Yes, rev1 is used by Xtables-addons.
Indeed. It was Jan who recommended this to be changed to 2 to avoid
conflict with Xtables-addons.
--
Cheers,
Luca.
^ permalink raw reply
* Re: Remaining BKL users, what to do
From: Arnd Bergmann @ 2010-09-21 20:01 UTC (permalink / raw)
To: Petr Vandrovec
Cc: Anton Altaparmakov, Jan Kara, codalist, autofs, linux-media,
dri-devel, Christoph Hellwig, Mikulas Patocka, Trond Myklebust,
Anders Larsen, Evgeniy Dushistov, Ingo Molnar, netdev,
Samuel Ortiz, Arnaldo Carvalho de Melo, linux-kernel,
linux-fsdevel, Andrew Hendry
In-Reply-To: <ead83d0d-e0ae-456d-8702-16ed8d3a179a@email.android.com>
On Saturday 18 September 2010 01:21:41 Petr Vandrovec wrote:
>
> I'll try to come up with something for ncpfs.
Ok, good.
> Trivial lock replacement will open deadlock possibility when
> someone reads to page which is also mmaped from the same
> filesystem (like grep likes to do). BKL with its automated
> release on sleep helped (or papered over) a lot here.
Right, I was more or less expecting something like this.
So I guess this is some AB-BA deadlock with another mutex
or a call to flush_scheduled_work that is currently done
under the BKL?
There is still the possibility of just working around those
by adding explicit mutex_unlock() calls around those, which
is what I initially did in the tty subsystem. The better
long-term approach would obviously be to understand all of
the data structures that actually need locking and only
lock the actual accesses, but that may be more work than
you are willing to spend on it.
Arnd
^ permalink raw reply
* Re: [PATCH RESEND 1/3] netfilter: xtables: inclusion of xt_condition
From: Luciano Coelho @ 2010-09-21 20:02 UTC (permalink / raw)
To: ext Patrick McHardy
Cc: Jan Engelhardt, netfilter-devel@vger.kernel.org,
netdev@vger.kernel.org, sameo@linux.intel.com,
Ylalehto Janne (Nokia-MS/Tampere)
In-Reply-To: <4C91AD41.1020802@trash.net>
On Thu, 2010-09-16 at 07:38 +0200, ext Patrick McHardy wrote:
> On 15.09.2010 23:09, Jan Engelhardt wrote:
> >
> > On Wednesday 2010-09-15 22:59, Patrick McHardy wrote:
> >> On 15.09.2010 22:52, Jan Engelhardt wrote:
> >>>
> >>> On Wednesday 2010-09-15 22:39, Patrick McHardy wrote:
> >>>> Am 17.08.2010 10:36, schrieb Luciano Coelho:
> >>>>> +/* Defaults, these can be overridden on the module command-line. */
> >>>>> +static unsigned int condition_list_perms = S_IRUGO | S_IWUSR;
> >>>>> +static unsigned int condition_uid_perms = 0;
> >>>>> +static unsigned int condition_gid_perms = 0;
> >>>>
> >>>> I'm not sure whether we already discussed this, but this isn't
> >>>> useful if namespaces are used since the IDs aren't global.
> >>>
> >>> Well it's also done for other xt modules that offer a procfs interface.
> >>> It does not really cost much to have this present I'd say.
> >>
> >> They should simply set the permissions for the current namespace.
> >>
> > IIRC, the namespace stuff was going to be a later patch to keep
> > early patches simpler.
>
> I'm fine with that as long as its done before merging the patches
> upstream.
I'll try to fix this and create a new patch on top of the existing ones,
then I'll send them all as a single patchset.
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH] netxen: dont set skb->truesize
From: David Miller @ 2010-09-21 20:04 UTC (permalink / raw)
To: eric.dumazet; +Cc: amit.salecha, netdev, ameen.rahman, anirban.chakraborty
In-Reply-To: <1285098910.2452.9.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 21 Sep 2010 21:55:10 +0200
> BTW, drivers/net/netxen/netxen_nic_init.c has same problem.
>
> [PATCH] netxen: dont set skb->truesize
Applied to net-2.6, and I backported the qlcnic patch in net-next-2.6
into net-2.6 too.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] xmit_compl_seq: information to reclaim vmsplice buffers
From: Rick Jones @ 2010-09-21 20:15 UTC (permalink / raw)
To: Tom Herbert; +Cc: netdev, davem, sridharr
In-Reply-To: <alpine.DEB.1.00.1009211154150.32565@pokey.mtv.corp.google.com>
> Using recvmsg data in this manner is sort of a cheap way to get a
> "callback" for when a vmspliced buffer is consumed. It will work
> well for a client where the response causes recvmsg to return.
> On the server side it works well if there are a sufficient
> number of requests coming on the connection (resorting to the
> timeout if necessary as described above).
If there is a recvmsg(), how often will the data received implicitly/explicitly
tell the application how much of the previously sent data has been acked? A
subsequent request from the client in a persistent (but not pipelined) HTTP
session implicitly says the data of the previous response was ACKed no? Is that
simply too rare to rely upon?
On the bulk side, an application filling the (fixed size at least) socket buffer
will "know" that the bytes sent a "socket buffer size ago" were ACKed because
that is what makes room in the socket buffer for data right?
rick jones
ftp://ftp.cup.hp.com/dist/networking/briefs/copyavoid.pdf
^ permalink raw reply
* de4x5 hangs with SMP: synchronize_irq() in interrupt handler?
From: Ondrej Zary @ 2010-09-21 20:30 UTC (permalink / raw)
To: netdev; +Cc: Kernel development list
Hello,
I'm trying to get two Compex FreedomLine 32 PnP-PCI2 cards to work (21041-AA
and 21041-PA) - there are problems with de2104x driver (one of the cards does
not work at all and the other one switches to non-existing AUI port when the
link goes down and never switches back to RJ45) and I know that de4x5 driver
worked for me in past.
Loading de4x5 causes the machine to hang immediately. It hangs at
synchronize_irq() call from de4x5_interrupt(). Commenting out this allows the
driver to work. Without SMP, synchronize_irq() is redefined to barrier() so
it works.
I don't have a clue how to fix this properly - does anyone know?
--
Ondrej Zary
^ permalink raw reply
* Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.
From: Kyle Moffett @ 2010-09-21 20:47 UTC (permalink / raw)
To: John Stultz
Cc: Richard Cochran, netdev, linux-kernel, linuxppc-dev,
devicetree-discuss, linux-arm-kernel, Krzysztof Halasa,
Rodolfo Giometti, Arnd Bergmann
In-Reply-To: <1282948239.2268.155.camel@jstultz-laptop>
On Fri, Aug 27, 2010 at 18:30, John Stultz <johnstul@us.ibm.com> wrote:
> On one side, there is the view that the kernel should abstract and hide
> the hardware details to increase app portability. And the other is that
> the raw hardware details should be exposed, so the OS stays out of the
> way.
>
> Neither of these views are always right. Ideally we can come up with a
> abstracted way to provide the hardware data needed that's generic enough
> that applications don't have to be changed when moving between hardware
> or configurations.
>
> The posix clock id interface is frustrating because the flat static
> enumeration is really limiting.
>
> I wonder if a dynamic enumeration for posix clocks would be possibly a
> way to go?
Well how about something much more straightforward:
#define CLOCK_FD 0x80000000
fd = open("/dev/clocks/some_clock_name", O_RDONLY);
clock_gettime(CLOCK_FD | fd, &ts);
This would provide all of the standard character-device semantics that
everyone is accustomed to, and furthermore you could allow non-root
users to manage specific clocks using just DAC permission bits.
You'd still probably need to add a clock_adjtimex() syscall, but it
could easily fail with EINVAL for software or per-thread clocks, but
that seems like it would nicely abstract the hardware without forcing
a numeric address space.
Cheers,
Kyle Moffett
^ permalink raw reply
* mv643xx pegasos breakage again
From: pacman @ 2010-09-21 21:06 UTC (permalink / raw)
To: netdev
Long ago, in another thread started by me,
Lennert Buytenhek wrote:
>Also, since the mv643xx_eth phylib conversion, it appears that the
>PHY on the Pegasos board fails to initialise properly, and this can
>apparently be worked around by turning the Marvell phylib driver off.
>Are you seeing this as well?
Gabriel Paubert answered:
>At least I experienced this problem. On the Pegasos, the generic
>PHY driver works, the Marvell specific ones screws up (at least
>when connected to a 100Mb/s hub, it detects it as 1Gb).
I didn't have anything to add at the time, having never actually tried the
marvell phy driver, and I quickly forgot about the issue. But now I've put a
2.6.35 kernel on a pegasos, and the marvell phy driver is being autoloaded.
The above symptom appears. Previously, it was possible to have the offending
driver harmlessly sitting in /lib/modules and as long as it wasn't loaded,
things were OK.
The misdetected speed is only the first symptom. No packets are ever
received, even after manually setting the speed.
Also during one experiment, the kernel allowed me to rmmod marvell and then
Oopsed, which I thought was a little rude.
It's still easy to work around: just don't build that driver.
Since this "new" problem was hard for me to identify (the marvell phy driver
doesn't announce itself in the kernel log, so there was no obvious sign that
a different driver was being loaded, and I had to git bisect to figure out
what happened between 2.6.34 and 2.6.35) I figure I should at least put my
solution on the record. And volunteer for further testing in case someone is
interested in digging deeper into why pegasos doesn't like marvell.ko.
^ permalink raw reply
* Deadlock in Bluetooth code in 2.6.36
From: Gustavo F. Padovan @ 2010-09-21 21:20 UTC (permalink / raw)
To: netdev; +Cc: linux-bluetooth, linux-kernel, padovan, marcel, davem, mathewm
Hi,
I would like to discuss here a problem we are having in the Bluetooth Layer.
There we have the Enhanced Retransmission Mode(ERTM) which is a reliable mode
of operation of the Bluetooth L2CAP layer. Think on it like a simplified
version of TCP.
The problem we were facing was a deadlock. ERTM uses a backlog queue to
queue incoming packets while the user held the lock. The problem is that
at some moment the user doesn't have memory anymore to alloc new skbs and
sleeps to wait for memory with the lock held, that stalls the ERTM connection
because we can't read the acknowledgements packets in the backlog queue to
free memory and then make the allocation of outcoming skb successful.
Looks like the solution to this issue is release the lock before go to sleep
waiting for memory and then lock the sock lock again when we wake up and have
memory. That's the solution TCP does through sk_stream_alloc_skb() and
sk_stream_wait_memory(). The sock lock is released and locked again inside
sk_wait_event() macro.
For bluetooth we do that in different way, we alloc memory through
sock_alloc_send_skb() which differently from sk_stream_alloc_skb() sleeps
with the lock held when waiting for memory.
The following patch is more like a proof of concept that sleeping
without the lock fixes the problem for L2CAP. Mat Martineau also found out
that some time ago.
My questions here is on how to fix this properly. Maybe
sock_alloc_send_skb() should not be used with SOCK_STREAM and reliable
protocols and I'm not aware of that? And should I use something like
sk_stream_alloc_skb() instead?
Any comments are welcome. With lucky we can fix that for 2.6.36 and
together with others fixes we have queued deliver a fully functional
L2CAP layer on 2.6.36.
Thanks,
---
Gustavo F. Padovan (1):
Bluetooth: Fix deadlock in the ERTM logic
net/bluetooth/l2cap.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
^ permalink raw reply
* [PATCH] Bluetooth: Fix deadlock in the ERTM logic
From: Gustavo F. Padovan @ 2010-09-21 21:20 UTC (permalink / raw)
To: netdev; +Cc: linux-bluetooth, linux-kernel, padovan, marcel, davem, mathewm
In-Reply-To: <1285104013-9946-1-git-send-email-padovan@profusion.mobi>
The Enhanced Retransmission Mode(ERTM) is a realiable mode of operation
of the Bluetooth L2CAP layer. Think on it like a simplified version of
TCP.
The problem we were facing here was a deadlock. ERTM uses a backlog
queue to queue incomimg packets while the user is helding the lock. The
problem is that at some moment the user doesn't have memory anymore to
do alloc new skbs and sleep with the lock to wait for memory, that
stalls the ERTM connection once we can't read the acknowledgements
packets in the backlog queue to free memory and make the allocation of
outcoming skb successful.
I'm thinking on this patch more like a proof of concept than a real fix
to the deadlock in ERTM.
Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
net/bluetooth/l2cap.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 44a8fb0..dd406b5 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1633,7 +1633,9 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
while (len) {
count = min_t(unsigned int, conn->mtu, len);
+ release_sock(sk);
*frag = bt_skb_send_alloc(sk, count, msg->msg_flags & MSG_DONTWAIT, &err);
+ lock_sock(sk);
if (!*frag)
return -EFAULT;
if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
@@ -1724,8 +1726,10 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
hlen += 2;
count = min_t(unsigned int, (conn->mtu - hlen), len);
+ release_sock(sk);
skb = bt_skb_send_alloc(sk, count + hlen,
msg->msg_flags & MSG_DONTWAIT, &err);
+ lock_sock(sk);
if (!skb)
return ERR_PTR(-ENOMEM);
--
1.7.3
^ permalink raw reply related
* Re: [PATCH v2] xmit_compl_seq: information to reclaim vmsplice buffers
From: Eric Dumazet @ 2010-09-21 21:38 UTC (permalink / raw)
To: Tom Herbert; +Cc: netdev, davem, sridharr
In-Reply-To: <alpine.DEB.1.00.1009211154150.32565@pokey.mtv.corp.google.com>
Le mardi 21 septembre 2010 à 11:57 -0700, Tom Herbert a écrit :
> In this patch we propose to adds some socket API to retrieve the
> "transmit completion sequence number", essentially a byte counter
> for the number of bytes that have been transmitted and will not be
> retransmitted. In the case of TCP, this should correspond to snd_una.
>
> The purpose of this API is to provide information to userspace about
> which buffers can be reclaimed when sending with vmsplice() on a
> socket.
>
> There are two methods for retrieving the completed sequence number:
> through a simple getsockopt (implemented here for TCP), as well as
> returning the value in the ancilary data of a recvmsg.
>
> The expected flow would be something like:
> - Connect is created
> - Initial completion seq # is retrieved through the sockopt, and is
> stored in userspace "compl_seq" variable for the connection.
> - Whenever a send is done, compl_seq += # bytes sent.
> - When doing a vmsplice the completion sequence number is saved
> for each user space buffer, buffer_compl_seq = compl_seq.
> - When recvmsg returns with a completion sequence number in
> ancillary data, any buffers cover by that sequence number
> (where buffer_compl_seq < recvmsg_compl_seq) are reclaimed
> and can be written to again.
> - If no data is receieved on a connection (recvmsg does not
> return), a timeout can be used to call the getsockopt and
> reclaim buffers as a fallback.
>
> Using recvmsg data in this manner is sort of a cheap way to get a
> "callback" for when a vmspliced buffer is consumed. It will work
> well for a client where the response causes recvmsg to return.
> On the server side it works well if there are a sufficient
> number of requests coming on the connection (resorting to the
> timeout if necessary as described above).
>
> Signed-off-by: Tom Herbert <therbert@google.com>
> + * Copy the first unacked seq into the receive msg control part.
> + */
> +static inline void tcp_sock_xmit_compl_seq(struct msghdr *msg,
> + struct sock *sk)
> +{
> + if (sock_flag(sk, SOCK_XMIT_COMPL_SEQ)) {
> + struct tcp_sock *tp = tcp_sk(sk);
> + if (msg->msg_controllen >= sizeof(tp->snd_una)) {
> + put_cmsg(msg, SOL_SOCKET, SCM_XMIT_COMPL_SEQ,
> + sizeof(tp->snd_una), &tp->snd_una);
> + }
> + }
> +}
I am wondering if this part could be done outside of socket lock,
provided you latch tp->snd_una value right before release_sock();
u32 snd_una;
...
tcp_cleanup_rbuf(sk, copied);
TCP_CHECK_TIMER(sk);
snd_una = tp->snd_una;
release_sock(sk);
tcp_sock_xmit_compl_seq(msg, sk, snd_una);
return copied;
^ permalink raw reply
* [PATCH net-2.6] caif: Remove buggy re-definition of pr_debug
From: sjur.brandeland @ 2010-09-21 20:40 UTC (permalink / raw)
To: davem; +Cc: netdev, Sjur Braendeland
From: Sjur Braendeland <sjur.brandeland@stericsson.com>
Remove debugging quirk redefining pr_debug to pr_warning.
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
Hi Dave,
I don't know if this qualify for going to 2.6.36.
It's not really a serious bug, but it would be
nice to get it fixed.
Regards,
Sjur
net/caif/chnl_net.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/net/caif/chnl_net.c b/net/caif/chnl_net.c
index 4293e19..73f81b5 100644
--- a/net/caif/chnl_net.c
+++ b/net/caif/chnl_net.c
@@ -28,9 +28,6 @@
#define CONNECT_TIMEOUT (5 * HZ)
#define CAIF_NET_DEFAULT_QUEUE_LEN 500
-#undef pr_debug
-#define pr_debug pr_warning
-
/*This list is protected by the rtnl lock. */
static LIST_HEAD(chnl_net_list);
--
1.6.3.3
^ permalink raw reply related
* [PATCH net-next-2.6 1/3] caif: Minor fixes in log prints.
From: sjur.brandeland @ 2010-09-21 21:44 UTC (permalink / raw)
To: davem; +Cc: netdev, Sjur Braendeland, Sjur Braendeland
From: Sjur Braendeland <sjur.braendeland@stericsson.com>
Use pr_debug for flow control printouts, and refine an error printout.
Signed-off-by: Sjur Braendeland <sjur.brandeland@stericsson.com>
---
net/caif/caif_socket.c | 8 ++------
net/caif/cfcnfg.c | 2 +-
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/net/caif/caif_socket.c b/net/caif/caif_socket.c
index fd1f5df..3943398 100644
--- a/net/caif/caif_socket.c
+++ b/net/caif/caif_socket.c
@@ -159,9 +159,7 @@ static int caif_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
if (atomic_read(&sk->sk_rmem_alloc) + skb->truesize >=
(unsigned)sk->sk_rcvbuf && rx_flow_is_on(cf_sk)) {
- trace_printk("CAIF: %s(): "
- "sending flow OFF (queue len = %d %d)\n",
- __func__,
+ pr_debug("sending flow OFF (queue len = %d %d)\n",
atomic_read(&cf_sk->sk.sk_rmem_alloc),
sk_rcvbuf_lowwater(cf_sk));
set_rx_flow_off(cf_sk);
@@ -174,9 +172,7 @@ static int caif_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
return err;
if (!sk_rmem_schedule(sk, skb->truesize) && rx_flow_is_on(cf_sk)) {
set_rx_flow_off(cf_sk);
- trace_printk("CAIF: %s(): "
- "sending flow OFF due to rmem_schedule\n",
- __func__);
+ pr_debug("sending flow OFF due to rmem_schedule\n");
dbfs_atomic_inc(&cnt.num_rx_flow_off);
caif_flow_ctrl(sk, CAIF_MODEMCMD_FLOW_OFF_REQ);
}
diff --git a/net/caif/cfcnfg.c b/net/caif/cfcnfg.c
index ef93a13..41adafd 100644
--- a/net/caif/cfcnfg.c
+++ b/net/caif/cfcnfg.c
@@ -197,7 +197,7 @@ int cfcnfg_disconn_adapt_layer(struct cfcnfg *cnfg, struct cflayer *adap_layer)
caif_assert(adap_layer != NULL);
channel_id = adap_layer->id;
if (adap_layer->dn == NULL || channel_id == 0) {
- pr_err("adap_layer->id is 0\n");
+ pr_err("adap_layer->dn == NULL or adap_layer->id is 0\n");
ret = -ENOTCONN;
goto end;
}
--
1.6.3.3
^ permalink raw reply related
* [PATCH net-next-2.6 2/3] caif: Fix function NULL pointer check.
From: sjur.brandeland @ 2010-09-21 21:44 UTC (permalink / raw)
To: davem; +Cc: netdev, Sjur Braendeland
In-Reply-To: <1285105486-7649-1-git-send-email-sjur.brandeland@stericsson.com>
From: Sjur Braendeland <sjur.brandeland@stericsson.com>
Check that receive function pointer is not null before calling it.
Signed-off-by: Sjur Braendeland <sjur.brandeland@stericsson.com>
---
net/caif/caif_dev.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index 0fd01dd..b99369a 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -173,7 +173,7 @@ static int receive(struct sk_buff *skb, struct net_device *dev,
net = dev_net(dev);
pkt = cfpkt_fromnative(CAIF_DIR_IN, skb);
caifd = caif_get(dev);
- if (!caifd || !caifd->layer.up || !caifd->layer.up->ctrlcmd)
+ if (!caifd || !caifd->layer.up || !caifd->layer.up->receive)
return NET_RX_DROP;
if (caifd->layer.up->receive(caifd->layer.up, pkt))
--
1.6.3.3
^ permalink raw reply related
* [PATCH net-next-2.6 3/3] caif: Use default send and receive buffer size in caif_socket.
From: sjur.brandeland @ 2010-09-21 21:44 UTC (permalink / raw)
To: davem; +Cc: netdev, Sjur Braendeland
In-Reply-To: <1285105486-7649-2-git-send-email-sjur.brandeland@stericsson.com>
From: Sjur Braendeland <sjur.brandeland@stericsson.com>
CAIF sockets should use socket's default send and receive buffers sizes.
Signed-off-by: Sjur Braendeland <sjur.brandeland@stericsson.com>
---
net/caif/caif_socket.c | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)
diff --git a/net/caif/caif_socket.c b/net/caif/caif_socket.c
index 3943398..4d918f8 100644
--- a/net/caif/caif_socket.c
+++ b/net/caif/caif_socket.c
@@ -30,9 +30,6 @@
MODULE_LICENSE("GPL");
MODULE_ALIAS_NETPROTO(AF_CAIF);
-#define CAIF_DEF_SNDBUF (4096*10)
-#define CAIF_DEF_RCVBUF (4096*100)
-
/*
* CAIF state is re-using the TCP socket states.
* caif_states stored in sk_state reflect the state as reported by
@@ -1118,10 +1115,6 @@ static int caif_create(struct net *net, struct socket *sock, int protocol,
/* Store the protocol */
sk->sk_protocol = (unsigned char) protocol;
- /* Sendbuf dictates the amount of outbound packets not yet sent */
- sk->sk_sndbuf = CAIF_DEF_SNDBUF;
- sk->sk_rcvbuf = CAIF_DEF_RCVBUF;
-
/*
* Lock in order to try to stop someone from opening the socket
* too early.
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH v2] xmit_compl_seq: information to reclaim vmsplice buffers
From: Eric Dumazet @ 2010-09-21 21:47 UTC (permalink / raw)
To: Tom Herbert; +Cc: netdev, davem, sridharr
In-Reply-To: <1285105131.6378.19.camel@edumazet-laptop>
Le mardi 21 septembre 2010 à 23:39 +0200, Eric Dumazet a écrit :
> > + * Copy the first unacked seq into the receive msg control part.
> > + */
> > +static inline void tcp_sock_xmit_compl_seq(struct msghdr *msg,
> > + struct sock *sk)
> > +{
> > + if (sock_flag(sk, SOCK_XMIT_COMPL_SEQ)) {
> > + struct tcp_sock *tp = tcp_sk(sk);
> > + if (msg->msg_controllen >= sizeof(tp->snd_una)) {
and this check is not necessary or correct ?
to put_cmsg() an u32, you need CMSG_LEN(4) bytes
> > + put_cmsg(msg, SOL_SOCKET, SCM_XMIT_COMPL_SEQ,
> > + sizeof(tp->snd_una), &tp->snd_una);
> > + }
> > + }
> > +}
->
if (sock_flag(sk, SOCK_XMIT_COMPL_SEQ))
put_cmsg(msg, SOL_SOCKET, SCM_XMIT_COMPL_SEQ,
sizeof(u32), &snd_una);
^ permalink raw reply
* (unknown)
From: gwurster @ 2010-09-21 20:59 UTC (permalink / raw)
>From ca1c566f51eeff4195b483addb86c6a73eaa37e0 Mon Sep 17 00:00:00 2001
From: Glenn Wurster <gwurster@scs.carleton.ca>
Date: Tue, 21 Sep 2010 16:59:00 -0400
Subject: [PATCH 2.6.36-rc3 1/1] IPv6: Create temporary address if none exists.
Cc: "David S. Miller" <davem@davemloft.net>,
Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
"Pekka Savola (ipv6)" <pekkas@netcore.fi>,
James Morris <jmorris@namei.org>,
Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
Patrick McHardy <kaber@trash.net>,
Stephen Hemminger <shemminger@vyatta.com>,
Eric Dumazet <eric.dumazet@gmail.com>,
Herbert Xu <herbert@gondor.apana.org.au>,
netdev@vger.kernel.org
To: linux-kernel@vger.kernel.org
X-Length: 1674
X-UID: 10
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Message-Id: <201009211659.02014.gwurster@scs.carleton.ca>
If privacy extentions are enabled, but no current temporary address exists,
then create one when we get a router advertisement.
Signed-off-by: Glenn Wurster <gwurster@scs.carleton.ca>
---
net/ipv6/addrconf.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ab70a3f..cfee6ae 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2022,10 +2022,11 @@ ok:
ipv6_ifa_notify(0, ift);
}
- if (create && in6_dev->cnf.use_tempaddr > 0) {
+ if ((create || list_empty(&in6_dev->tempaddr_list)) &&
in6_dev->cnf.use_tempaddr > 0) {
/*
* When a new public address is created as described in [ADDRCONF],
- * also create a new temporary address.
+ * also create a new temporary address. Also create a temporary
+ * address if it's enabled but no temporary address currently exists.
*/
read_unlock_bh(&in6_dev->lock);
ipv6_create_tempaddr(ifp, NULL);
^ permalink raw reply related
* Re: [PATCH net-next-2.6 7/8] sfc: Set net_device::num_rx_queues once we know the correct value
From: David Miller @ 2010-09-21 21:57 UTC (permalink / raw)
To: bhutchings; +Cc: eric.dumazet, netdev, linux-net-drivers
In-Reply-To: <1285009546.2282.126.camel@achroite.uk.solarflarecom.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 20 Sep 2010 20:05:46 +0100
> If RPS is enabled there's a separate kobject for each RX queue. Those
> other drivers probably should be setting it.
>
> Oh, but this only exists if CONFIG_RPS is enabled. I think we need an
> inline function for setting this.
It's set in the core by alloc_netdev_mq(), you should never have to
set this in your driver.
And that also takes care of the CONFIG_RPS dependency in one spot,
another good argument for drivers never touching this value.
I'm not applying this patch.
^ permalink raw reply
* Re: [PATCH net-next-2.6 0/8] sfc and ethtool changes for 2.6.37
From: David Miller @ 2010-09-21 21:58 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1285008056.2282.103.camel@achroite.uk.solarflarecom.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 20 Sep 2010 19:40:56 +0100
> This series adds Ethernet-level filtering and explicit filter clearing
> to the ethtool RX n-tuple interface, and implements it in the sfc
> driver.
>
> There is a cleanup patch on the end which is preparation for the
> following RFC patch series but is worthwhile anyway.
>
> Ben Hutchings (8):
> ethtool: Define RX n-tuple action to clear a rule
> ethtool: Add Ethernet MAC-level filtering/steering
> ethtool: Allocate register dump buffer with vmalloc()
> sfc: Add filter table management
> sfc: Implement the ethtool RX n-tuple control functions
> sfc: Include RX IP filter table in register dump
> sfc: Set net_device::num_rx_queues once we know the correct value
> sfc: Clean up and correct comments on efx_monitor()
All applied except patch #7 as noted in the thread for that patch.
Thanks!
^ 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