* Re: NET_NS: unregister_netdevice: waiting for lo to become free (after using openvpn)
From: Ben Greear @ 2010-08-05 16:57 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Michael Leun, Greg KH, netdev, davem, linux-kernel,
Alexey Dobriyan, Patrick McHardy
In-Reply-To: <m1y6clz8ta.fsf_-_@fess.ebiederm.org>
On 08/05/2010 02:51 AM, Eric W. Biederman wrote:
> Michael Leun<lkml20100708@newton.leun.net> writes:
>> Unfortunately looks like indefinitely. Never watched longer so far
>> (rebooted soon), but I'm seeing this message now repeated every 10 secs
>> for ~10 minutes on a idle system.
>
> Ugh. A real bug then. These can be a pain to track down and fix. I
> think the last one of these I tracked down took a couple of weeks. I
> will start digging in when I get back from vacation.
I once spent a similar amount of time putting in debug variants that
printed info for each time a netdev was acquired and released.
Maybe a similar logic could be put into the official kernel (and
disabled by default)?
That should save effort in the long run, I'd think.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: build failure
From: John Fastabend @ 2010-08-05 16:48 UTC (permalink / raw)
To: divya
Cc: LKML, linux-next@vger.kernel.org, linuxppc-dev@ozlabs.org, netdev,
Kirsher, Jeffrey T
In-Reply-To: <4C5A4CF4.4070203@linux.vnet.ibm.com>
divya wrote:
> Yestersday's linux-next(2.6.35_next_20100802) build fails with the following error on both system p and x.
>
>
> drivers/net/ixgbe/ixgbe_main.c: In function 'ixgbe_select_queue':
> drivers/net/ixgbe/ixgbe_main.c:6159: error: 'struct ixgbe_fcoe' has no member named 'up'
> drivers/net/ixgbe/ixgbe_main.c: In function 'ixgbe_xmit_frame':
> drivers/net/ixgbe/ixgbe_main.c:6221: error: 'struct ixgbe_fcoe' has no member named 'up'
> make[3]: *** [drivers/net/ixgbe/ixgbe_main.o] Error 1
> make[2]: *** [drivers/net/ixgbe] Error 2
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [drivers/net] Error 2
> make: *** [drivers] Error 2
>
> Thanks
> Divya
>
>
Hi Divya,
Jeff should have a fix for this in his queue already. This is caused by
building with CONFIG_FCOE and without CONFIG_IXGBE_DCB.
Thanks,
John.
^ permalink raw reply
* RE: Receive processing stops when dev->poll returns 1
From: Usha Srinivasan @ 2010-08-05 16:36 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev@vger.kernel.org
In-Reply-To: <20100805092243.41671259@nehalam>
I have compared the code in my driver to code in other drivers and they are quite similar. Here is my code:
int vnic_napi_poll(struct napi_struct *napi, int budget)
{
done = 0;
poll_more:
while (done < budget) {
int max = (budget - done);
t = min(<max-supported-by-driver>, max);
n = get-completions(comp_list);
for (i = 0; i < n; i++, done++)
handle_completions(<complist[i]);
if (n != t)
break;
}
if (done < budget) {
netif_rx_complete(dev, napi);
/* check again just to be sure */
if (more-completions()) {
If netif_rx_reschedule(dev, napi))
goto poll_more;
}
}
return done;
}
***********************
BACKPORTED version:
***********************
int vnic_poll(struct net_device *dev, int *budget)
{
int max = min(*budget, dev->quota);
done = 0;
poll_more:
while (max) {
t = min(<max-supported-by-driver>, max);
n = get-completions(comp_list);
for (i = 0; i < n; i++, --max, done++)
handle_completions(<complist[i]);
if (n != t)
break;
}
if (max) {
netif_rx_complete(dev);
/* check again just to be sure */
if (more-completions()) {
If netif_rx_reschedule(dev, napi))
goto poll_more;
}
ret = 0;
} else
ret = 1;
dev->quota -= done;
*budget -= done;
return ret;
}
***********************
-----Original Message-----
From: Stephen Hemminger [mailto:shemminger@vyatta.com]
Sent: Thursday, August 05, 2010 12:23 PM
To: Usha Srinivasan
Cc: netdev@vger.kernel.org
Subject: Re: Receive processing stops when dev->poll returns 1
On Thu, 5 Aug 2010 11:11:51 -0500
Usha Srinivasan <usha.srinivasan@qlogic.com> wrote:
> Thanks for your response. What you said is exactly what my driver is doing:
>
>
> <= 2.6.23
> Calls netif_rx_complete if done < budget; decrements quota & *budget by done; returns 0 if done < budget and 1 otherwise.
>
> When 1 is returned, I encounter the problem I described)
>
> > 2.6.23
> Calls napi-complete if done < budget; returns done.
>
> When done==budget, I encounter the problem I described.
>
> Any ideas?
Ignore last mail...
If you done == budget, the poll will be recalled (after other drivers).
If quantum exhausts, then it gets called it gets deferred to ksoftirq
thread.
One possibility is that the driver is looking at wrong parameter
for budget and is exceeding the requested value. Please post your code.
^ permalink raw reply
* Re: [PATCH] platform: Facilitate the creation of pseduo-platform busses
From: Patrick Pannuto @ 2010-08-05 16:31 UTC (permalink / raw)
To: Kevin Hilman
Cc: linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-omap@vger.kernel.org, damm@opensource.se,
lethal@linux-sh.org, rjw@sisk.pl, eric.y.miao@gmail.com,
netdev@vger.kernel.org, Greg Kroah-Hartman, alan, zt.tmzt,
grant.likely, magnus.damm
In-Reply-To: <87fwytxdba.fsf@deeprootsystems.com>
>>>> [snip]
>>>>
>>>> Which will allow the same driver to easily to used on either
>>>> the platform bus or the newly defined bus type.
>>>
>>> Except it requires a re-compile.
>>>
>>> Rather than doing this at compile time, it would be better to support
>>> legacy devices at runtime. You could handle this by simply registering
>>> the driver on the custom bus and the platform_bus and let the bus
>>> matching code handle it. Then, the same binary would work on both
>>> legacy and updated SoCs.
>>>
>>
>> Can you safely register a driver on more than one bus? I didn't think
>> that was safe -- normally it's impossible since you're calling
>>
>> struct BUS_TYPE_driver mydriver;
>> BUS_TYPE_driver_register(&mydriver)
>>
>> but now we have multiple "bus types" that are all actually platform type; still,
>> at a minimum you would need:
>> struct platform_driver mydrvier1 = {
>> .driver.bus = &sub_bus1,
>> };
>> struct platform_driver mydrvier2 = {
>> .driver.bus = &sub_bus2,
>> };
>> which would all point to the same driver functions, yet the respective devices
>> attached for the "same" driver would be on different buses. I fear this might
>> confuse some drivers. I don't think dynamic bus assignment is this easy
>>
>> In short: I do not believe the same driver can be registered on multiple
>> different buses -- if this is wrong, please correct me.
>
> It is possible, and currently done in powerpc land where some
> drivers handle devices on the platform_bus and the custom OF bus.
>
> However, as noted by Magnus, what we really need here is a way for
> drivers to not care at all what kind of bus they are on. There are an
> increasing number of drivers that are re-used not just across different
> SoCs in the same family, but across totally different SoCs (e.g. drivers
> for hardware shared between TI OMAP and TI DaVinci, or SH and SH-Mobile/ARM)
>
I will start trying to work on this
>>>
>>> Up to here, this looks exactly what I wrote in thread referenced
>>> above.
>>>
>>
>> It is, you just went on vacation :)
>>
>
> Ah, OK. The changelog was missing credits to that affect, but I was
> more concerned that you hadn't seen my example and didn't want to be
> duplicating work.
>
will fix.
>>>> [snip]
>
>> if you call it second then they will all already be well-defined and
>> thus not overwritten.
>
> Right, they will not be overwritten, but you'll be left with a mostly
> empty dev_pm_ops on the custom bus.
>
> IOW, Most of these custom busses will only want to customize a small
> subset of the dev_pm_ops methods (e.g. only the runtime PM methods.) If
> you setup your sparsly populated custom dev_pm_ops and then call
> platform_bus_type_init() second, dev_pm_ops on the new buswill have *only*
> your custom fields, and none of the defaults from platform_dev_pm_ops.
>
> So, what I was getting at is that it should probably be clearer to the
> users of platform_bus_type_init() that any customization of dev_pm_ops
> should be done after.
>
I understand what you're saying now, and I can fix this as well.
>
>>
>> If you would like to lead this effort, please do so; I did not mean to step
>> on your toes, it's just that this is an issue for me as well.
>
> No worries there, my toes are fine. :)
Good :)
>
>> You had indicated that you were going on vacation for a month and I
>> had not seen any more follow-up on this issue, so I forged ahead.
>
> Great, I'm glad you forged ahead. There is definitely a broader need
> for something like this, and I have no personal attachment to the code.
>
> I have no problems with you continuing the work (in fact, I'd prefer it.
> I have lots of other things to catch up on after my vacation.)
>
> In the future though, it's common (and kind) to note the original author
> in the changelog when basing a patch on previous work. Something like
> "originally written by..." or "based on the work of..." etc.
Ok, I can do that; that was the intention of the "original inspiration from"
line at the beginning. Is there a more formal way of indicating this in the
next version of the patch? Should I add you as a "From:" or an "Author:"?
-Pat
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply
* Re: [rfc 10/13] [RFC 10/13] IPVS: management of persistence engine modules
From: Stephen Hemminger @ 2010-08-05 16:29 UTC (permalink / raw)
To: Simon Horman
Cc: lvs-devel, netdev, netfilter, netfilter-devel, Wensong Zhang,
Julian Anastasov, Patrick McHardy
In-Reply-To: <20100805115241.482096330@vergenet.net>
On Thu, 05 Aug 2010 20:48:05 +0900
Simon Horman <horms@verge.net.au> wrote:
> +/* lock for service table */
> +static DEFINE_RWLOCK(__ip_vs_pe_lock);
It is already static so why the __?
Reader/writer locks are slower than spinlocks. Either use
a spinlock, or RCU (if possible)
> +/* Bind a service with a pe */
> +void ip_vs_bind_pe(struct ip_vs_service *svc, struct ip_vs_pe *pe)
> +{
> + svc->pe = pe;
> +}
> +
> +/* Unbind a service from its pe */
> +void ip_vs_unbind_pe(struct ip_vs_service *svc)
> +{
> + svc->pe = NULL;
> +}
What does having these wrappers buy?
^ permalink raw reply
* Re: [RFC PATCH 2/2] igb/ixgbe: add code to trigger function reset if reset_devices is set
From: David Woodhouse @ 2010-08-05 16:27 UTC (permalink / raw)
To: David Miller
Cc: jeffrey.t.kirsher, jbarnes, netdev, linux-pci, alexander.h.duyck
In-Reply-To: <20100801.011516.191407437.davem@davemloft.net>
On Sun, 2010-08-01 at 01:15 -0700, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Fri, 30 Jul 2010 17:59:12 -0700
>
> > From: Alexander Duyck <alexander.h.duyck@intel.com>
> >
> > This change makes it so that both igb and ixgbe can trigger a full pcie
> > function reset if the reset_devices kernel parameter is defined. The main
> > reason for adding this is that kdump can cause serious issues when the
> > kdump kernel resets the IOMMU while DMA transactions are still occurring.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>
> I tend to disagree with the essence of this change.
>
> Which is that we should add workaround after workaround for things
> that aren't functioning properly in kdump and kexec.
>
> They should have a pass that shuts devices down properly, so that this
> kind of stuff doesn't need to happen in the kernel we then boot into.
For a normal kexec, arguably true.
But in the kdump case, the original kernel has *crashed* and we really
don't have that option -- we need to jump *straight* to the new kernel
and have it reset the hardware.
The device driver really *ought* to be able to reset the hardware from
whatever state it's in when the new kernel starts up. Anything less is
broken, and reminds me of those crappy drivers that only work after a
soft-reboot from Windows.
Most drivers *do* quite happily initialise their device and reliably get
it into a known state; it's just that this particular hardware goes into
a *particularly* stroppy fit when it gets a DMA master abort (which is
what happens when the IOMMU stops it from scribbling into memory after
the new kernel has taken over).
> What happens on non-PCIE systems then? Do they just lose when this
> happens?
If they have a device that's this broken, and the driver can't get it
into a working state any other way, then yes -- I don't see any way to
*avoid* them losing.
I don't like the reset_devices thing though -- the device driver ought
to cope (and reset the device with a full PCIe reset if that's the only
way to make it stop sulking) *regardless* of that option, if it's
necessary.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
^ permalink raw reply
* Re: Receive processing stops when dev->poll returns 1
From: Stephen Hemminger @ 2010-08-05 16:22 UTC (permalink / raw)
To: Usha Srinivasan; +Cc: netdev@vger.kernel.org
In-Reply-To: <7866DA1F8D2D4541B87FEE88E633ABAA2B72081FDF@MNEXMB1.qlogic.org>
On Thu, 5 Aug 2010 11:11:51 -0500
Usha Srinivasan <usha.srinivasan@qlogic.com> wrote:
> Thanks for your response. What you said is exactly what my driver is doing:
>
>
> <= 2.6.23
> Calls netif_rx_complete if done < budget; decrements quota & *budget by done; returns 0 if done < budget and 1 otherwise.
>
> When 1 is returned, I encounter the problem I described)
>
> > 2.6.23
> Calls napi-complete if done < budget; returns done.
>
> When done==budget, I encounter the problem I described.
>
> Any ideas?
Ignore last mail...
If you done == budget, the poll will be recalled (after other drivers).
If quantum exhausts, then it gets called it gets deferred to ksoftirq
thread.
One possibility is that the driver is looking at wrong parameter
for budget and is exceeding the requested value. Please post your code.
^ permalink raw reply
* Re: Receive processing stops when dev->poll returns 1
From: Stephen Hemminger @ 2010-08-05 16:16 UTC (permalink / raw)
To: Usha Srinivasan; +Cc: netdev@vger.kernel.org
In-Reply-To: <7866DA1F8D2D4541B87FEE88E633ABAA2B72081FDF@MNEXMB1.qlogic.org>
On Thu, 5 Aug 2010 11:11:51 -0500
Usha Srinivasan <usha.srinivasan@qlogic.com> wrote:
> Thanks for your response. What you said is exactly what my driver is doing:
>
>
> <= 2.6.23
> Calls netif_rx_complete if done < budget; decrements quota & *budget by done; returns 0 if done < budget and 1 otherwise.
>
> When 1 is returned, I encounter the problem I described)
>
> > 2.6.23
> Calls napi-complete if done < budget; returns done.
>
> When done==budget, I encounter the problem I described.
>
Your driver did not call napi_complete (and re-enable interrupts).
^ permalink raw reply
* RE: Receive processing stops when dev->poll returns 1
From: Usha Srinivasan @ 2010-08-05 16:11 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev@vger.kernel.org
In-Reply-To: <20100805090447.3f67d990@nehalam>
Thanks for your response. What you said is exactly what my driver is doing:
<= 2.6.23
Calls netif_rx_complete if done < budget; decrements quota & *budget by done; returns 0 if done < budget and 1 otherwise.
When 1 is returned, I encounter the problem I described)
> 2.6.23
Calls napi-complete if done < budget; returns done.
When done==budget, I encounter the problem I described.
Any ideas?
-----Original Message-----
From: Stephen Hemminger [mailto:shemminger@vyatta.com]
Sent: Thursday, August 05, 2010 12:05 PM
To: Usha Srinivasan
Cc: netdev@vger.kernel.org
Subject: Re: Receive processing stops when dev->poll returns 1
On Thu, 5 Aug 2010 09:20:03 -0500
Usha Srinivasan <usha.srinivasan@qlogic.com> wrote:
> Hello,
> I have run into an interesting and frustrating problem which I've not been able to resolve. I am hoping someone can help me.
>
> I have a network driver which sets its dev->weight to 100 (like ipoib) and when it processes 100 received packets, following the rules, it decrements dev->quota and *budget and returns 1 without calling netif_rx_complete. When my driver does that, all processing of incoming packets for all interfaces comes to a halt.
>
> How do I know this? Because, as soon as my driver returns 1 to dev->poll, I lose my putty session and eth0 stops working; eth0 counters show that it stops receiving packets, though it is able to transmit. My own device stops receiving packets. I have scoured the code for ipoib and other network devices and I see no difference in what my driver does. I have tried to lower weight for ipoib & eth0 hoping to reproduce with those device it but no luck.
You maybe looking at old documentation on how NAPI works.
In NAPI <= 2.6.23, the driver changed dev->quota and budget
and returned 0 or 1.
For current kernels, the NAPI poll has changed.
Using your example,
dev->weight = 100
budget would be 100
if your network driver process 100 packets, it should return 100
and call napi_complete().
^ permalink raw reply
* Re: Receive processing stops when dev->poll returns 1
From: Stephen Hemminger @ 2010-08-05 16:04 UTC (permalink / raw)
To: Usha Srinivasan; +Cc: netdev@vger.kernel.org
In-Reply-To: <7866DA1F8D2D4541B87FEE88E633ABAA2B72081FB6@MNEXMB1.qlogic.org>
On Thu, 5 Aug 2010 09:20:03 -0500
Usha Srinivasan <usha.srinivasan@qlogic.com> wrote:
> Hello,
> I have run into an interesting and frustrating problem which I've not been able to resolve. I am hoping someone can help me.
>
> I have a network driver which sets its dev->weight to 100 (like ipoib) and when it processes 100 received packets, following the rules, it decrements dev->quota and *budget and returns 1 without calling netif_rx_complete. When my driver does that, all processing of incoming packets for all interfaces comes to a halt.
>
> How do I know this? Because, as soon as my driver returns 1 to dev->poll, I lose my putty session and eth0 stops working; eth0 counters show that it stops receiving packets, though it is able to transmit. My own device stops receiving packets. I have scoured the code for ipoib and other network devices and I see no difference in what my driver does. I have tried to lower weight for ipoib & eth0 hoping to reproduce with those device it but no luck.
You maybe looking at old documentation on how NAPI works.
In NAPI <= 2.6.23, the driver changed dev->quota and budget
and returned 0 or 1.
For current kernels, the NAPI poll has changed.
Using your example,
dev->weight = 100
budget would be 100
if your network driver process 100 packets, it should return 100
and call napi_complete().
^ permalink raw reply
* Re: [PATCH] platform: Facilitate the creation of pseduo-platform busses
From: Kevin Hilman @ 2010-08-05 15:57 UTC (permalink / raw)
To: Patrick Pannuto
Cc: linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-omap@vger.kernel.org, damm@opensource.se,
lethal@linux-sh.org, rjw@sisk.pl, eric.y.miao@gmail.com,
netdev@vger.kernel.org, Greg Kroah-Hartman, alan, zt.tmzt,
grant.likely, magnus.damm
In-Reply-To: <4C5A0C68.9080500@codeaurora.org>
Patrick Pannuto <ppannuto@codeaurora.org> writes:
> On 08/04/2010 05:16 PM, Kevin Hilman wrote:
>> Patrick Pannuto <ppannuto@codeaurora.org> writes:
>>
>>> Inspiration for this comes from:
>>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html
>>
>> Also, later in that thread I also wrote[1] what seems to be the core of
>> what you've done here: namely, allow platform_devices and
>> platform_drivers to to be used on custom busses. Patch is at the end of
>> this mail with a more focused changelog. As Greg suggested in his reply
>> to your first version, this part could be merged today, and the
>> platform_bus_init stuff could be added later, after some more review.
>> Some comments below...
>>
>
> I can split this into 2 patches.
Yes, I think that would be better.
> Was your patch sent to linux-kernel or just linux-omap? I'm not on linux-omap...
That thread was on linux-arm-kernel and linux-omap
>
>>> [snip]
>>>
>>> Which will allow the same driver to easily to used on either
>>> the platform bus or the newly defined bus type.
>>
>> Except it requires a re-compile.
>>
>> Rather than doing this at compile time, it would be better to support
>> legacy devices at runtime. You could handle this by simply registering
>> the driver on the custom bus and the platform_bus and let the bus
>> matching code handle it. Then, the same binary would work on both
>> legacy and updated SoCs.
>>
>
> Can you safely register a driver on more than one bus? I didn't think
> that was safe -- normally it's impossible since you're calling
>
> struct BUS_TYPE_driver mydriver;
> BUS_TYPE_driver_register(&mydriver)
>
> but now we have multiple "bus types" that are all actually platform type; still,
> at a minimum you would need:
> struct platform_driver mydrvier1 = {
> .driver.bus = &sub_bus1,
> };
> struct platform_driver mydrvier2 = {
> .driver.bus = &sub_bus2,
> };
> which would all point to the same driver functions, yet the respective devices
> attached for the "same" driver would be on different buses. I fear this might
> confuse some drivers. I don't think dynamic bus assignment is this easy
>
> In short: I do not believe the same driver can be registered on multiple
> different buses -- if this is wrong, please correct me.
It is possible, and currently done in powerpc land where some
drivers handle devices on the platform_bus and the custom OF bus.
However, as noted by Magnus, what we really need here is a way for
drivers to not care at all what kind of bus they are on. There are an
increasing number of drivers that are re-used not just across different
SoCs in the same family, but across totally different SoCs (e.g. drivers
for hardware shared between TI OMAP and TI DaVinci, or SH and SH-Mobile/ARM)
>>
>> Up to here, this looks exactly what I wrote in thread referenced
>> above.
>>
>
> It is, you just went on vacation :)
>
Ah, OK. The changelog was missing credits to that affect, but I was
more concerned that you hadn't seen my example and didn't want to be
duplicating work.
>>>
>>> if (code != retval)
>>> platform_driver_unregister(drv);
>>> @@ -1017,6 +1019,26 @@ struct bus_type platform_bus_type = {
>>> };
>>> EXPORT_SYMBOL_GPL(platform_bus_type);
>>>
>>> +/** platform_bus_type_init - fill in a pseudo-platform-bus
>>> + * @bus: foriegn bus type
>>> + *
>>> + * This init is basically a selective memcpy that
>>> + * won't overwrite any user-defined attributes and
>>> + * only copies things that platform bus defines anyway
>>> + */
>>
>> minor nit: kernel doc style has wrong indentation
>>
>
> will fix
>
>>> +void platform_bus_type_init(struct bus_type *bus)
>>> +{
>>> + if (!bus->dev_attrs)
>>> + bus->dev_attrs = platform_bus_type.dev_attrs;
>>> + if (!bus->match)
>>> + bus->match = platform_bus_type.match;
>>> + if (!bus->uevent)
>>> + bus->uevent = platform_bus_type.uevent;
>>> + if (!bus->pm)
>>> + bus->pm = platform_bus_type.pm;
>>> +}
>>> +EXPORT_SYMBOL_GPL(platform_bus_type_init);
>>
>> With this approach, you should note in the comments/changelog that
>> any selective customization of the bus PM methods must be done after
>> calling platform_bus_type_init().
>
> No they don't. If you call platform_bus_type_init first then you'll
> just overwrite them with new values;
Yes.
> if you call it second then they will all already be well-defined and
> thus not overwritten.
Right, they will not be overwritten, but you'll be left with a mostly
empty dev_pm_ops on the custom bus.
IOW, Most of these custom busses will only want to customize a small
subset of the dev_pm_ops methods (e.g. only the runtime PM methods.) If
you setup your sparsly populated custom dev_pm_ops and then call
platform_bus_type_init() second, dev_pm_ops on the new buswill have *only*
your custom fields, and none of the defaults from platform_dev_pm_ops.
So, what I was getting at is that it should probably be clearer to the
users of platform_bus_type_init() that any customization of dev_pm_ops
should be done after.
>>
>> Also, You've left out the legacy PM methods here. That implies that
>> moving a driver from the platform_bus to the custom bus is not entirely
>> transparent. If the driver still has legacy PM methods, it would stop
>> working on the custom bus.
>>
>> While this is good motivation for converting a driver to dev_pm_ops, at
>> a minimum it should be clear in the changelog that the derivative busses
>> do not support legacy PM methods. However, since it's quite easy to do,
>> and you want the derivative busses to be *exactly* like the platform bus
>> except where explicitly changed, I'd suggest you also check/copy the
>> legacy PM methods.
>>
>> In addition, you've missed several fields in 'struct bus_type'
>> (bus_attr, drv_attr, p, etc.) Without digging deeper into the driver
>> core, I'm not sure if they are all needed at init time, but it should be
>> clear in the comments why they can be excluded.
>>
>
> I copied everything that was defined for platform_bus_type:
>
> struct bus_type platform_bus_type = {
> .name = "platform",
> .dev_attrs = platform_dev_attrs,
> .match = platform_match,
> .uevent = platform_uevent,
> .pm = &platform_dev_pm_ops,
> };
> EXPORT_SYMBOL_GPL(platform_bus_type);
>
> struct bus_type {
> const char *name;
> struct bus_attribute *bus_attrs;
> struct device_attribute *dev_attrs;
> struct driver_attribute *drv_attrs;
>
> int (*match)(struct device *dev, struct device_driver *drv);
> int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
> int (*probe)(struct device *dev);
> int (*remove)(struct device *dev);
> void (*shutdown)(struct device *dev);
>
> int (*suspend)(struct device *dev, pm_message_t state);
> int (*resume)(struct device *dev);
>
> const struct dev_pm_ops *pm;
>
> struct bus_type_private *p;
> };
>
> It is my understanding that everything that I did not copy *should* remain
> unique to each bus; remaining fields will be filled in by bus_register and
> should not be copied.
>
[...]
>
> If you would like to lead this effort, please do so; I did not mean to step
> on your toes, it's just that this is an issue for me as well.
No worries there, my toes are fine. :)
> You had indicated that you were going on vacation for a month and I
> had not seen any more follow-up on this issue, so I forged ahead.
Great, I'm glad you forged ahead. There is definitely a broader need
for something like this, and I have no personal attachment to the code.
I have no problems with you continuing the work (in fact, I'd prefer it.
I have lots of other things to catch up on after my vacation.)
In the future though, it's common (and kind) to note the original author
in the changelog when basing a patch on previous work. Something like
"originally written by..." or "based on the work of..." etc.
Thanks,
Kevin
^ permalink raw reply
* [PATCH] cfg80211: support sysfs namespaces
From: Johannes Berg @ 2010-08-05 15:45 UTC (permalink / raw)
To: John Linville; +Cc: linux-wireless, netdev, Eric W. Biederman
From: Johannes Berg <johannes.berg@intel.com>
Enable using network namespaces with
wireless devices even when sysfs is
enabled using the same infrastructure
that was built for netdevs.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
NB: This generates a harmless gcc warning, I submitted
a patch to fix it here:
http://thread.gmane.org/gmane.linux.kernel/1019330
include/linux/netdevice.h | 2 ++
net/core/net-sysfs.c | 3 ++-
net/wireless/core.c | 7 ++++++-
net/wireless/sysfs.c | 9 +++++++++
4 files changed, 19 insertions(+), 2 deletions(-)
--- wireless-testing.orig/net/core/net-sysfs.c 2010-08-05 17:33:47.000000000 +0200
+++ wireless-testing/net/core/net-sysfs.c 2010-08-05 17:38:27.000000000 +0200
@@ -789,12 +789,13 @@ static const void *net_netlink_ns(struct
return sock_net(sk);
}
-static struct kobj_ns_type_operations net_ns_type_operations = {
+struct kobj_ns_type_operations net_ns_type_operations = {
.type = KOBJ_NS_TYPE_NET,
.current_ns = net_current_ns,
.netlink_ns = net_netlink_ns,
.initial_ns = net_initial_ns,
};
+EXPORT_SYMBOL_GPL(net_ns_type_operations);
static void net_kobj_ns_exit(struct net *net)
{
--- wireless-testing.orig/net/wireless/sysfs.c 2010-08-05 17:33:47.000000000 +0200
+++ wireless-testing/net/wireless/sysfs.c 2010-08-05 17:39:11.000000000 +0200
@@ -110,6 +110,13 @@ static int wiphy_resume(struct device *d
return ret;
}
+static const void *wiphy_namespace(struct device *d)
+{
+ struct wiphy *wiphy = container_of(d, struct wiphy, dev);
+
+ return wiphy_net(wiphy);
+}
+
struct class ieee80211_class = {
.name = "ieee80211",
.owner = THIS_MODULE,
@@ -120,6 +127,8 @@ struct class ieee80211_class = {
#endif
.suspend = wiphy_suspend,
.resume = wiphy_resume,
+ .ns_type = &net_ns_type_operations,
+ .namespace = wiphy_namespace,
};
int wiphy_sysfs_init(void)
--- wireless-testing.orig/net/wireless/core.c 2010-08-05 17:33:47.000000000 +0200
+++ wireless-testing/net/wireless/core.c 2010-08-05 17:38:27.000000000 +0200
@@ -253,11 +253,16 @@ int cfg80211_switch_netns(struct cfg8021
WARN_ON(err);
wdev->netdev->features |= NETIF_F_NETNS_LOCAL;
}
+
+ return err;
}
wiphy_net_set(&rdev->wiphy, net);
- return err;
+ err = device_rename(&rdev->wiphy.dev, dev_name(&rdev->wiphy.dev));
+ WARN_ON(err);
+
+ return 0;
}
static void cfg80211_rfkill_poll(struct rfkill *rfkill, void *data)
--- wireless-testing.orig/include/linux/netdevice.h 2010-08-05 17:13:50.000000000 +0200
+++ wireless-testing/include/linux/netdevice.h 2010-08-05 17:39:02.000000000 +0200
@@ -2171,6 +2171,8 @@ extern void dev_seq_stop(struct seq_file
extern int netdev_class_create_file(struct class_attribute *class_attr);
extern void netdev_class_remove_file(struct class_attribute *class_attr);
+extern struct kobj_ns_type_operations net_ns_type_operations;
+
extern char *netdev_drivername(const struct net_device *dev, char *buffer, int len);
extern void linkwatch_run_queue(void);
^ permalink raw reply
* Re: [PATCH] platform: Facilitate the creation of pseduo-platform busses
From: Kevin Hilman @ 2010-08-05 15:27 UTC (permalink / raw)
To: Magnus Damm
Cc: Patrick Pannuto, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, linux-omap@vger.kernel.org,
damm@opensource.se, lethal@linux-sh.org, rjw@sisk.pl,
eric.y.miao@gmail.com, netdev@vger.kernel.org, Greg Kroah-Hartman,
alan, zt.tmzt
In-Reply-To: <AANLkTimkMy6_201XNGb41e7LivkvVHMkFZWVoA-7UmBo@mail.gmail.com>
Magnus Damm <magnus.damm@gmail.com> writes:
> On Thu, Aug 5, 2010 at 7:14 AM, Patrick Pannuto <ppannuto@codeaurora.org> wrote:
>> Inspiration for this comes from:
>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31161.html
>>
>> RFC: http://lkml.org/lkml/2010/8/3/496
>> Patch is unchanged from the RFC. Reviews seemed generally positive
>> and it seemed this was desired functionality.
>
> Thanks for your patch, it's really nice to see work done in this area!
> I'd like to see something like this merged in the not so distant
> future. At this point I'm not so concerned about the details, so I'll
> restrict myself to this:
>
>> /drivers/my_driver.c
>> static struct platform_driver my_driver = {
>> .driver = {
>> .name = "my-driver",
>> .owner = THIS_MODULE,
>> .bus = &my_bus_type,
>> },
>> };
>
> I would really prefer not to have the bus type in the here. I
> understand it's needed at this point, but I wonder if it's possible to
> adjust the device<->driver matching for platform devices to allow any
> type of pseudo-platform bus_type.
I totally agree here. Keeping the drivers ignorant of the bus (or SoC)
they are on will make them much more portable.
Kevin
^ permalink raw reply
* CONFIG_FIXED_PHY breaks fec on arm/mx51
From: Uwe Kleine-König @ 2010-08-05 15:17 UTC (permalink / raw)
To: netdev; +Cc: Bryan Wu, Andy Whitcroft, Amit Kucheria, Stefan Bader
Hello,
when I enable CONFIG_FIXED_PHY my mx51evm board fails to initialize the
fec network device. The relevant kernel log is:
...
Fixed MDIO Bus: probed
FEC Ethernet Driver
------------[ cut here ]------------
WARNING: at /ptx/work/octopus/WORK_2_A/ukl/backup/gsrc/linux-2.6/fs/sysfs/dir.c:451 sysfs_add_one+0x6c/0x8c()
sysfs: cannot create duplicate filename '/class/mdio_bus/0'
Modules linked in:
Backtrace:
[<c0026484>] (dump_backtrace+0x0/0x10c) from [<c02b6158>] (dump_stack+0x18/0x1c)
r7:df839d60 r6:c00db380 r5:c03434df r4:000001c3
[<c02b6140>] (dump_stack+0x0/0x1c) from [<c003750c>] (warn_slowpath_common+0x58/0x88)
[<c00374b4>] (warn_slowpath_common+0x0/0x88) from [<c00375e0>] (warn_slowpath_fmt+0x38/0x40)
r8:df84a480 r7:ffffffef r6:df839db0 r5:df811000 r4:df8dc600
[<c00375a8>] (warn_slowpath_fmt+0x0/0x40) from [<c00db380>] (sysfs_add_one+0x6c/0x8c)
r3:df811000 r2:c0343523
[<c00db314>] (sysfs_add_one+0x0/0x8c) from [<c00dc398>] (sysfs_do_create_link+0xfc/0x1c8)
r7:df8e8648 r6:00000000 r5:df8dc600 r4:df8dc570
[<c00dc29c>] (sysfs_do_create_link+0x0/0x1c8) from [<c00dc490>] (sysfs_create_link+0x14/0x18)
[<c00dc47c>] (sysfs_create_link+0x0/0x18) from [<c01e1918>] (device_add+0x198/0x520)
[<c01e1780>] (device_add+0x0/0x520) from [<c01e1cc0>] (device_register+0x20/0x24)
[<c01e1ca0>] (device_register+0x0/0x24) from [<c01ffbfc>] (mdiobus_register+0x90/0x178)
r4:df8e8600
[<c01ffb6c>] (mdiobus_register+0x0/0x178) from [<c02b5f50>] (fec_probe+0x39c/0x48c)
[<c02b5bb4>] (fec_probe+0x0/0x48c) from [<c01e4fb4>] (platform_drv_probe+0x20/0x24)
[<c01e4f94>] (platform_drv_probe+0x0/0x24) from [<c01e3ec8>] (driver_probe_device+0xb0/0x188)
[<c01e3e18>] (driver_probe_device+0x0/0x188) from [<c01e400c>] (__driver_attach+0x6c/0x90)
r7:df839f00 r6:c03abe0x58/0x98)
r6:c03abe08 r5:c01e3fa0 r4:00000000
[<c01e3668>] (bus_for_each_dev+0x0/0x98) from [<c01e3d24>] (driver_attach+0x20/0x28)
r7:df8db1e0 r6:c03abe08 r5:c03abe08 r4:c001e498
[<c01e3d04>] (driver_attach+0x0/0x28) from [<c01e2f2c>] (bus_add_driver+0xa4/0x228)
[<c01e2e88>] (bus_add_driver+0x0/0x228) from [<c01e4338>] (driver_register+0xc8/0x154)
[<c01e4270>] (driver_register+0x0/0x154) from [<c01e5410>] (platform_driver_register+0x4c/0x60)
r8:00000000 r7:00000000 r6:c00187a8 r5:c001e4f8 r4:c001e498
[<c01e53c4>] (platform_driver_register+0x0/0x60) from [<c00187c4>] (fec_enet_module_init+0x1c/0x28)
[<c00187a8>] (fec_enet_module_init+0x0/0x28) from [<c0022384>] (do_one_initcall+0x64/0x1c4)
[<c0022320>] (do_one_initcall+0x0/0x1c4) from [<c00085f4>] (kernel_init+0xa0/0x154)
r7:00000013 r6:c003a7dc r5:c001e4f8 r4:c001e498
[<c0008554>] (kernel_init+0x0/0x154) from [<c003a7dc>] (do_exit+0x0/0x564)
r5:c0008554 r4:00000000
---[ end trace 1220371fd8e97d24 ]---
mii_bus 0 failed to register
...
As far as I understand the problem this is just that both drivers
(drivers/net/phy/fixed.c and drivers/net/fec.c) try to register a
mdio_bus with the same number. There isn't a mechanism to circumvent
this problem, e.g. some auto-increment logic that just isn't used, is
there?
The people from Ubuntu had the same problem (Cc:d):
http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-lucid.git;a=commitdiff;h=0e5e6e2a981eeab61dcc184d51ab769a33af6589
(I used Linus' tree as of today (3a09b1be53d2) plus a few unrelated
patches with mx51_defconfig.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 2/2] netfilter: xt_condition: change the value from boolean to u32
From: Jan Engelhardt @ 2010-08-05 15:12 UTC (permalink / raw)
To: luciano.coelho; +Cc: netfilter-devel, netdev, kaber, sameo
In-Reply-To: <1281019269-30985-3-git-send-email-luciano.coelho@nokia.com>
On Thursday 2010-08-05 16:41, luciano.coelho@nokia.com wrote:
> struct xt_condition_mtinfo {
>- char name[31];
>+ char name[27];
> __u8 invert;
>+ __u32 value;
Please also bump the .revision field to 2 with this patch so that
testing can always proceed without an ABI clash.
(rev 2 would then remain over the course of the remaining patches
you submit.)
(rev 0 = ipt_condition/pom-ng; rev 1 = xt_condition from Xt-a)
>+ char buf[14];
char buf[sizeof("4294967296")];
seems more intuitive :-)
>+ unsigned long long value;
>+
>+ if (length == 0)
>+ return 0;
>+
>+ if (length > sizeof(buf))
>+ return -EINVAL;
>+
>+ if (copy_from_user(buf, input, length) != 0)
>+ return -EFAULT;
>+
>+ buf[length - 1] = '\0';
>+
>+ if (strict_strtoull(buf, 0, &value) != 0)
>+ return -EINVAL;
>+
>+ if (value > (u32) value)
>+ return -EINVAL;
Is it possible to use just strict_strtoul?
>- return var->enabled ^ info->invert;
>+ return (var->value == info->value) ^ info->invert;
Since the condition value (cdmark) was thought of an nfmark-style thing,
would it perhaps make sense to model it after it
return (var->value & ~info->mask) ^ info->value;
Other opinions?
^ permalink raw reply
* Re: [PATCH 12/15] net: sunrpc: removed duplicated #include
From: J. Bruce Fields @ 2010-08-05 15:09 UTC (permalink / raw)
To: Andrea Gelmini
Cc: gelma-PT7Us6ch6G9eoWH0uzbU5w, Trond Myklebust, Neil Brown,
David S. Miller, Frederic Weisbecker,
linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1281016299-23958-12-git-send-email-andrea.gelmini-PT7Us6ch6G9eoWH0uzbU5w@public.gmane.org>
On Thu, Aug 05, 2010 at 03:51:36PM +0200, Andrea Gelmini wrote:
> Signed-off-by: Andrea Gelmini <andrea.gelmini-PT7Us6ch6G9eoWH0uzbU5w@public.gmane.org>
Thanks, applied for 2.6.36.
--b.
> ---
> net/sunrpc/cache.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index 58de76c..b75879b 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -34,7 +34,6 @@
> #include <linux/sunrpc/cache.h>
> #include <linux/sunrpc/stats.h>
> #include <linux/sunrpc/rpc_pipe_fs.h>
> -#include <linux/smp_lock.h>
>
> #define RPCDBG_FACILITY RPCDBG_CACHE
>
> --
> 1.7.2.1.85.g2d089
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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] [Bug 16494] NFS client over TCP hangs due to packet loss
From: Andy Chittenden @ 2010-08-05 14:55 UTC (permalink / raw)
To: 'Andy Chittenden', 'Andrew Morton'
Cc: 'David Miller', kuznet-v/Mj1YrvjDBInbfyfbPRSQ,
pekkas-UjJjq++bwZ7HOG6cAo2yLw, jmorris-gx6/JNMH7DfYtjvyW6yDsg,
yoshfuji-VfPWfsRibaP+Ru+s062T9g, kaber-dcUjhNyLwpNeoWH0uzbU5w,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
William.Allen.Simpson-Re5JQEeQqe8AvxtiuMwx3w,
gilad-f4XOiQkOAtcdH0auuBZGHA,
ilpo.jarvinen-pxSi+dnQzZMxHbG02/KK1g,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-nfs-u79uwXL29TY76Z2rM5mHXA, 'Trond Myklebust',
'J. Bruce Fields', 'Neil Brown',
'Chuck Lever', 'Benny Halevy',
'Alexandros Batsakis', 'Joe Perches'
In-Reply-To: <4C57EE9A.7040308-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> On 2010-08-03 10:11, Andrew Morton wrote:
> > (cc linux-nfs)
> >
> > On Tue, 03 Aug 2010 01:21:44 -0700 (PDT) David
> Miller<davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> >
> >> From: "Andy Chittenden"<andyc.bluearc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Date: Tue, 3 Aug 2010 09:14:31 +0100
> >>
> >>> I don't know whether this patch is the correct fix or not but it
> enables the
> >>> NFS client to recover.
> >>>
> >>> Kernel version: 2.6.34.1 and 2.6.32.
> >>>
> >>> Fixes<https://bugzilla.kernel.org/show_bug.cgi?id=16494>. It clears
> down
> >>> any previous shutdown attempts so that reconnects on a socket
> that's been
> >>> shutdown leave the socket in a usable state (otherwise
> tcp_sendmsg() returns
> >>> -EPIPE).
> >>
> >> If the SunRPC code wants to close a TCP socket then use it again,
> >> it should disconnect by doing a connect() with sa_family ==
> AF_UNSPEC
>
> There is code to do that in the SunRPC code in xs_abort_connection()
> but
> that's conditionally called from xs_tcp_reuse_connection():
>
> static void xs_tcp_reuse_connection(struct rpc_xprt *xprt, struct
> sock_xprt *transport)
> {
> unsigned int state = transport->inet->sk_state;
>
> if (state == TCP_CLOSE && transport->sock->state ==
> SS_UNCONNECTED)
> return;
> if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
> return;
> xs_abort_connection(xprt, transport);
> }
>
> That's changed since 2.6.26 where it unconditionally did the connect()
> with sa_family == AF_UNSPEC. FWIW we cannot reproduce this problem with
> 2.6.26.
The problem is fixed with this patch which also prints out that sk_shutdown
can be non-zero on entry to xs_tcp_reuse_connection:
# diff -up /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c
net/sunrpc/xprtsock.c
--- /home/company/software/src/linux-2.6.34.2/net/sunrpc/xprtsock.c
2010-08-02 18:30:51.000000000 +0100
+++ net/sunrpc/xprtsock.c 2010-08-05 12:21:11.000000000 +0100
@@ -1322,10 +1322,11 @@ static void xs_tcp_state_change(struct s
if (!(xprt = xprt_from_sock(sk)))
goto out;
dprintk("RPC: xs_tcp_state_change client %p...\n", xprt);
- dprintk("RPC: state %x conn %d dead %d zapped %d\n",
+ dprintk("RPC: state %x conn %d dead %d zapped %d sk_shutdown
%d\n",
sk->sk_state, xprt_connected(xprt),
sock_flag(sk, SOCK_DEAD),
- sock_flag(sk, SOCK_ZAPPED));
+ sock_flag(sk, SOCK_ZAPPED),
+ sk->sk_shutdown);
switch (sk->sk_state) {
case TCP_ESTABLISHED:
@@ -1796,10 +1797,18 @@ static void xs_tcp_reuse_connection(stru
{
unsigned int state = transport->inet->sk_state;
- if (state == TCP_CLOSE && transport->sock->state == SS_UNCONNECTED)
- return;
- if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT))
- return;
+ if (state == TCP_CLOSE && transport->sock->state == SS_UNCONNECTED)
{
+ if (transport->inet->sk_shutdown == 0)
+ return;
+ printk("%s: TCP_CLOSEd and sk_shutdown set to %d\n",
+ __func__, transport->inet->sk_shutdown);
+ }
+ if ((1 << state) & (TCPF_ESTABLISHED|TCPF_SYN_SENT)) {
+ if (transport->inet->sk_shutdown == 0)
+ return;
+ printk("%s: sk_shutdown set to %d\n",
+ __func__, transport->inet->sk_shutdown);
+ }
xs_abort_connection(xprt, transport);
}
Signed-off-by: Andy Chittenden <andyc.bluearc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
dmesg displays:
[ 2840.896043] xs_tcp_reuse_connection: TCP_CLOSEd and sk_shutdown set to 2
so previously the code was attempting to reuse the connection but wasn't
aborting it and thus didn't clear down sk_shutdown.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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: GIT: net-*2.6 rebased...
From: Arnd Hannemann @ 2010-08-05 14:45 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-wireless, sfr
In-Reply-To: <20100805.014637.13722703.davem@davemloft.net>
Am 05.08.2010 10:46, schrieb David Miller:
> From: Arnd Hannemann <arnd@arndnet.de> Date: Thu, 05 Aug 2010
> 09:57:50 +0200
>
>> if I try to fetch from net-next-2.6, I get: fatal: The remote end
>> hung up unexpectedly
>>
>> if I try to clone net-2.6: (e.g. git clone
>> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-2.6.git
>>
>> I get lots of errors reading the tags "does not point to a valid
>> object!" and then: remote: error: Could not read
>> 3cfc2c42c1cbc8e238bb9c0612c0df4565e3a8b4 remote: fatal: Failed to
>> traverse parents of commit
>> 7aaaaa1e44b2a4047dfe05f304a5090eb995cf44 remote: aborting due to
>> possible repository corruption on the remote side. fatal: early
>> EOF fatal: index-pack failed
>
> I fixed the alternates file but it will take some time for the fix to
> propagate from master.kernel.org
>
> Let me know if it doesn't work even after an hour or so.
It works now, for both net-next-2.6 and net-2.6.
Thanks,
Arnd
^ permalink raw reply
* Re: [PATCH 11/15] net: mac80211: Fix a typo.
From: Gábor Stefanik @ 2010-08-05 14:44 UTC (permalink / raw)
To: Andrea Gelmini
Cc: gelma, Stefano Brivio, Mattias Nissler, John W. Linville,
Johannes Berg, David S. Miller, Stephen Hemminger,
Alexey Dobriyan, Tejun Heo, linux-wireless, netdev
In-Reply-To: <1281016299-23958-11-git-send-email-andrea.gelmini@gelma.net>
On Thu, Aug 5, 2010 at 3:51 PM, Andrea Gelmini <andrea.gelmini@gelma.net> wrote:
> "userpace" -> "userspace"
>
> Signed-off-by: Andrea Gelmini <andrea.gelmini@gelma.net>
> ---
> net/mac80211/rc80211_pid_debugfs.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/mac80211/rc80211_pid_debugfs.c b/net/mac80211/rc80211_pid_debugfs.c
> index 47438b4..135f36f 100644
> --- a/net/mac80211/rc80211_pid_debugfs.c
> +++ b/net/mac80211/rc80211_pid_debugfs.c
> @@ -162,7 +162,7 @@ static ssize_t rate_control_pid_events_read(struct file *file, char __user *buf,
> file_info->next_entry = (file_info->next_entry + 1) %
> RC_PID_EVENT_RING_SIZE;
>
> - /* Print information about the event. Note that userpace needs to
> + /* Print information about the event. Note that userspace needs to
> * provide large enough buffers. */
> length = length < RC_PID_PRINT_BUF_SIZE ?
> length : RC_PID_PRINT_BUF_SIZE;
length = min(length, RC_PID_PRINT_BUF_SIZE) maybe? (Though this is not
part of the patch.)
> --
> 1.7.2.1.85.g2d089
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)
^ permalink raw reply
* [PATCH 0/2] netfilter: xtables: xt_condition inclusion and change to u32
From: luciano.coelho @ 2010-08-05 14:41 UTC (permalink / raw)
To: netfilter-devel; +Cc: netdev, kaber, jengelh, sameo
From: Luciano Coelho <luciano.coelho@nokia.com>
Hi again,
Here is the xt_condition patch, hopefully for inclusion this time.
When these patches get accepted, I'll send the implementation of the condition
target, as discussed before.
In RFC v2 I've made a few changes as discussed in the review:
* Removed per-netns module parameters
* Use par->net instead of current->nsproxy->net_ns
* Fix file-leak in procfs when exiting the netns
I didn't get any more comments in RFC v2, so I assume it is okay to send it for
inclusion.
>From [RFC v2] to [PATCH], I've only rebased and added a new patch to support
u32 instead of boolean as the value for the condition.
Cheers,
Luca.
PS: I've been on vacation, that's why it took sometime to submit this patch again.
Luciano Coelho (2):
netfilter: xtables: inclusion of xt_condition
netfilter: xt_condition: change the value from boolean to u32
include/linux/netfilter/Kbuild | 1 +
include/linux/netfilter/xt_condition.h | 15 ++
net/netfilter/Kconfig | 8 +
net/netfilter/Makefile | 1 +
net/netfilter/xt_condition.c | 268 ++++++++++++++++++++++++++++++++
5 files changed, 293 insertions(+), 0 deletions(-)
create mode 100644 include/linux/netfilter/xt_condition.h
create mode 100644 net/netfilter/xt_condition.c
^ permalink raw reply
* [PATCH 2/2] netfilter: xt_condition: change the value from boolean to u32
From: luciano.coelho @ 2010-08-05 14:41 UTC (permalink / raw)
To: netfilter-devel; +Cc: netdev, kaber, jengelh, sameo
In-Reply-To: <1281019269-30985-1-git-send-email-luciano.coelho@nokia.com>
From: Luciano Coelho <luciano.coelho@nokia.com>
Previously the condition match was using boolean values for the condition,
which is a bit limited, so this patch changes the value to a u32.
This is not a problem at the moment, since only userspace can set the
condition and it can do more advanced checks before setting the value. But
when the condition target gets implemented, having a u32 value will make it
much more flexible.
Signed-off-by: Luciano Coelho <luciano.coelho@nokia.com>
---
include/linux/netfilter/xt_condition.h | 3 +-
net/netfilter/xt_condition.c | 51 +++++++++++++++++---------------
2 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/include/linux/netfilter/xt_condition.h b/include/linux/netfilter/xt_condition.h
index 4faf3ca..c4fe899 100644
--- a/include/linux/netfilter/xt_condition.h
+++ b/include/linux/netfilter/xt_condition.h
@@ -4,8 +4,9 @@
#include <linux/types.h>
struct xt_condition_mtinfo {
- char name[31];
+ char name[27];
__u8 invert;
+ __u32 value;
/* Used internally by the kernel */
void *condvar __attribute__((aligned(8)));
diff --git a/net/netfilter/xt_condition.c b/net/netfilter/xt_condition.c
index a78d832..08dfb67 100644
--- a/net/netfilter/xt_condition.c
+++ b/net/netfilter/xt_condition.c
@@ -48,7 +48,7 @@ struct condition_variable {
struct list_head list;
struct proc_dir_entry *status_proc;
unsigned int refcount;
- bool enabled;
+ u32 value;
};
struct condition_net {
@@ -71,32 +71,35 @@ static int condition_proc_read(char __user *buffer, char **start, off_t offset,
{
const struct condition_variable *var = data;
- buffer[0] = var->enabled ? '1' : '0';
- buffer[1] = '\n';
- if (length >= 2)
- *eof = true;
- return 2;
+ return snprintf(buffer, length, "%u\n", var->value);
}
-static int condition_proc_write(struct file *file, const char __user *buffer,
+static int condition_proc_write(struct file *file, const char __user *input,
unsigned long length, void *data)
{
struct condition_variable *var = data;
- char newval;
-
- if (length > 0) {
- if (get_user(newval, buffer) != 0)
- return -EFAULT;
- /* Match only on the first character */
- switch (newval) {
- case '0':
- var->enabled = false;
- break;
- case '1':
- var->enabled = true;
- break;
- }
- }
+ char buf[14];
+ unsigned long long value;
+
+ if (length == 0)
+ return 0;
+
+ if (length > sizeof(buf))
+ return -EINVAL;
+
+ if (copy_from_user(buf, input, length) != 0)
+ return -EFAULT;
+
+ buf[length - 1] = '\0';
+
+ if (strict_strtoull(buf, 0, &value) != 0)
+ return -EINVAL;
+
+ if (value > (u32) value)
+ return -EINVAL;
+
+ var->value = value;
+
return length;
}
@@ -106,7 +109,7 @@ condition_mt(const struct sk_buff *skb, struct xt_action_param *par)
const struct xt_condition_mtinfo *info = par->matchinfo;
const struct condition_variable *var = info->condvar;
- return var->enabled ^ info->invert;
+ return (var->value == info->value) ^ info->invert;
}
static int condition_mt_check(const struct xt_mtchk_param *par)
@@ -156,7 +159,7 @@ static int condition_mt_check(const struct xt_mtchk_param *par)
}
var->refcount = 1;
- var->enabled = false;
+ var->value = 0;
var->status_proc->data = var;
var->status_proc->read_proc = condition_proc_read;
var->status_proc->write_proc = condition_proc_write;
--
1.7.0.4
^ permalink raw reply related
* [PATCH 1/2] netfilter: xtables: inclusion of xt_condition
From: luciano.coelho @ 2010-08-05 14:41 UTC (permalink / raw)
To: netfilter-devel; +Cc: netdev, kaber, jengelh, sameo
In-Reply-To: <1281019269-30985-1-git-send-email-luciano.coelho@nokia.com>
From: Luciano Coelho <luciano.coelho@nokia.com>
xt_condition can be used by userspace to influence decisions in rules
by means of togglable variables without having to reload the entire
ruleset.
This is a respin of the module in Xtables-addons, with support for
multiple namespaces and other small improvements. Some of the changes
were made by Jan Engelhardt.
Cc: Jan Engelhardt <jengelh@medozas.de>
Signed-off-by: Luciano Coelho <luciano.coelho@nokia.com>
---
include/linux/netfilter/Kbuild | 1 +
include/linux/netfilter/xt_condition.h | 14 ++
net/netfilter/Kconfig | 8 +
net/netfilter/Makefile | 1 +
net/netfilter/xt_condition.c | 265 ++++++++++++++++++++++++++++++++
5 files changed, 289 insertions(+), 0 deletions(-)
create mode 100644 include/linux/netfilter/xt_condition.h
create mode 100644 net/netfilter/xt_condition.c
diff --git a/include/linux/netfilter/Kbuild b/include/linux/netfilter/Kbuild
index edeeabd..60363f5 100644
--- a/include/linux/netfilter/Kbuild
+++ b/include/linux/netfilter/Kbuild
@@ -22,6 +22,7 @@ header-y += xt_TEE.h
header-y += xt_TPROXY.h
header-y += xt_cluster.h
header-y += xt_comment.h
+header-y += xt_condition.h
header-y += xt_connbytes.h
header-y += xt_connlimit.h
header-y += xt_connmark.h
diff --git a/include/linux/netfilter/xt_condition.h b/include/linux/netfilter/xt_condition.h
new file mode 100644
index 0000000..4faf3ca
--- /dev/null
+++ b/include/linux/netfilter/xt_condition.h
@@ -0,0 +1,14 @@
+#ifndef _XT_CONDITION_H
+#define _XT_CONDITION_H
+
+#include <linux/types.h>
+
+struct xt_condition_mtinfo {
+ char name[31];
+ __u8 invert;
+
+ /* Used internally by the kernel */
+ void *condvar __attribute__((aligned(8)));
+};
+
+#endif /* _XT_CONDITION_H */
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index 4328825..5044dd6 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -621,6 +621,14 @@ config NETFILTER_XT_MATCH_COMMENT
If you want to compile it as a module, say M here and read
<file:Documentation/kbuild/modules.txt>. If unsure, say `N'.
+config NETFILTER_XT_MATCH_CONDITION
+ tristate '"condition" match support'
+ depends on NETFILTER_ADVANCED
+ depends on PROC_FS
+ ---help---
+ This option allows you to match firewall rules against condition
+ variables stored in the /proc/net/nf_condition directory.
+
config NETFILTER_XT_MATCH_CONNBYTES
tristate '"connbytes" per-connection counter match support'
depends on NF_CONNTRACK
diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile
index 441050f..bbf72bb 100644
--- a/net/netfilter/Makefile
+++ b/net/netfilter/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_NETFILTER_XT_TARGET_IDLETIMER) += xt_IDLETIMER.o
# matches
obj-$(CONFIG_NETFILTER_XT_MATCH_CLUSTER) += xt_cluster.o
obj-$(CONFIG_NETFILTER_XT_MATCH_COMMENT) += xt_comment.o
+obj-$(CONFIG_NETFILTER_XT_MATCH_CONDITION) += xt_condition.o
obj-$(CONFIG_NETFILTER_XT_MATCH_CONNBYTES) += xt_connbytes.o
obj-$(CONFIG_NETFILTER_XT_MATCH_CONNLIMIT) += xt_connlimit.o
obj-$(CONFIG_NETFILTER_XT_MATCH_CONNTRACK) += xt_conntrack.o
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;
+
+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");
+
+struct condition_variable {
+ struct list_head list;
+ struct proc_dir_entry *status_proc;
+ unsigned int refcount;
+ bool enabled;
+};
+
+struct condition_net {
+ struct list_head list;
+ struct proc_dir_entry *proc_dir;
+};
+
+static int condition_net_id;
+static inline struct condition_net *condition_pernet(struct net *net)
+{
+ return net_generic(net, condition_net_id);
+}
+
+/* proc_lock is a user context only semaphore used for write access */
+/* to the conditions' list. */
+static DEFINE_MUTEX(proc_lock);
+
+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';
+ if (length >= 2)
+ *eof = true;
+ return 2;
+}
+
+static int condition_proc_write(struct file *file, const char __user *buffer,
+ unsigned long length, void *data)
+{
+ struct condition_variable *var = data;
+ char newval;
+
+ if (length > 0) {
+ if (get_user(newval, buffer) != 0)
+ return -EFAULT;
+ /* Match only on the first character */
+ switch (newval) {
+ case '0':
+ var->enabled = false;
+ break;
+ case '1':
+ var->enabled = true;
+ break;
+ }
+ }
+ return length;
+}
+
+static bool
+condition_mt(const struct sk_buff *skb, struct xt_action_param *par)
+{
+ const struct xt_condition_mtinfo *info = par->matchinfo;
+ const struct condition_variable *var = info->condvar;
+
+ return var->enabled ^ info->invert;
+}
+
+static int condition_mt_check(const struct xt_mtchk_param *par)
+{
+ struct xt_condition_mtinfo *info = par->matchinfo;
+ struct condition_variable *var;
+ struct condition_net *cond_net = condition_pernet(par->net);
+
+ /* Forbid certain names */
+ if (*info->name == '\0' || *info->name == '.' ||
+ info->name[sizeof(info->name)-1] != '\0' ||
+ memchr(info->name, '/', sizeof(info->name)) != NULL) {
+ pr_info("name not allowed or too long: \"%.*s\"\n",
+ (unsigned int)sizeof(info->name), info->name);
+ return -EINVAL;
+ }
+
+ /*
+ * Let's acquire the lock, check for the condition and add it
+ * or increase the reference counter.
+ */
+ mutex_lock(&proc_lock);
+ list_for_each_entry(var, &cond_net->list, list) {
+ if (strcmp(info->name, var->status_proc->name) == 0) {
+ ++var->refcount;
+ mutex_unlock(&proc_lock);
+ info->condvar = var;
+ return 0;
+ }
+ }
+
+ /* At this point, we need to allocate a new condition variable. */
+ var = kmalloc(sizeof(struct condition_variable), GFP_KERNEL);
+ if (var == NULL) {
+ mutex_unlock(&proc_lock);
+ return -ENOMEM;
+ }
+
+ /* Create the condition variable's proc file entry. */
+ var->status_proc = create_proc_entry(info->name,
+ condition_list_perms,
+ cond_net->proc_dir);
+ if (var->status_proc == NULL) {
+ kfree(var);
+ mutex_unlock(&proc_lock);
+ return -ENOMEM;
+ }
+
+ var->refcount = 1;
+ var->enabled = false;
+ var->status_proc->data = var;
+ var->status_proc->read_proc = condition_proc_read;
+ var->status_proc->write_proc = condition_proc_write;
+ var->status_proc->uid = condition_uid_perms;
+ var->status_proc->gid = condition_gid_perms;
+ list_add(&var->list, &cond_net->list);
+ mutex_unlock(&proc_lock);
+ info->condvar = var;
+ return 0;
+}
+
+static void condition_mt_destroy(const struct xt_mtdtor_param *par)
+{
+ const struct xt_condition_mtinfo *info = par->matchinfo;
+ struct condition_variable *var = info->condvar;
+ struct condition_net *cond_net = condition_pernet(par->net);
+
+ mutex_lock(&proc_lock);
+ if (--var->refcount == 0) {
+ list_del(&var->list);
+ /* status_proc may be null in case of ns exit */
+ if (var->status_proc)
+ remove_proc_entry(var->status_proc->name,
+ cond_net->proc_dir);
+ mutex_unlock(&proc_lock);
+ kfree(var);
+ return;
+ }
+ mutex_unlock(&proc_lock);
+}
+
+static struct xt_match condition_mt_reg __read_mostly = {
+ .name = "condition",
+ .revision = 1,
+ .family = NFPROTO_UNSPEC,
+ .matchsize = sizeof(struct xt_condition_mtinfo),
+ .match = condition_mt,
+ .checkentry = condition_mt_check,
+ .destroy = condition_mt_destroy,
+ .me = THIS_MODULE,
+};
+
+static const char *const dir_name = "nf_condition";
+
+static int __net_init condnet_mt_init(struct net *net)
+{
+ struct condition_net *cond_net = condition_pernet(net);
+
+ INIT_LIST_HEAD(&cond_net->list);
+
+ cond_net->proc_dir = proc_mkdir(dir_name, net->proc_net);
+
+ return (cond_net->proc_dir == NULL) ? -EACCES : 0;
+}
+
+static void __net_exit condnet_mt_exit(struct net *net)
+{
+ struct condition_net *cond_net = condition_pernet(net);
+ struct condition_variable *var, *tmp;
+
+ mutex_lock(&proc_lock);
+ list_for_each_entry_safe(var, tmp, &cond_net->list, list) {
+ remove_proc_entry(var->status_proc->name,
+ cond_net->proc_dir);
+ /* set to null so we don't double remove in mt_destroy */
+ var->status_proc = NULL;
+ }
+
+ mutex_unlock(&proc_lock);
+
+ remove_proc_entry(dir_name, net->proc_net);
+}
+
+static struct pernet_operations condition_mt_netops = {
+ .init = condnet_mt_init,
+ .exit = condnet_mt_exit,
+ .id = &condition_net_id,
+ .size = sizeof(struct condition_net),
+};
+
+static int __init condition_mt_init(void)
+{
+ int ret;
+
+ mutex_init(&proc_lock);
+ ret = xt_register_match(&condition_mt_reg);
+ if (ret < 0)
+ return ret;
+
+ ret = register_pernet_subsys(&condition_mt_netops);
+ if (ret < 0) {
+ xt_unregister_match(&condition_mt_reg);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void __exit condition_mt_exit(void)
+{
+ unregister_pernet_subsys(&condition_mt_netops);
+ xt_unregister_match(&condition_mt_reg);
+}
+
+module_init(condition_mt_init);
+module_exit(condition_mt_exit);
--
1.7.0.4
^ permalink raw reply related
* Receive processing stops when dev->poll returns 1
From: Usha Srinivasan @ 2010-08-05 14:20 UTC (permalink / raw)
To: netdev@vger.kernel.org
Hello,
I have run into an interesting and frustrating problem which I've not been able to resolve. I am hoping someone can help me.
I have a network driver which sets its dev->weight to 100 (like ipoib) and when it processes 100 received packets, following the rules, it decrements dev->quota and *budget and returns 1 without calling netif_rx_complete. When my driver does that, all processing of incoming packets for all interfaces comes to a halt.
How do I know this? Because, as soon as my driver returns 1 to dev->poll, I lose my putty session and eth0 stops working; eth0 counters show that it stops receiving packets, though it is able to transmit. My own device stops receiving packets. I have scoured the code for ipoib and other network devices and I see no difference in what my driver does. I have tried to lower weight for ipoib & eth0 hoping to reproduce with those device it but no luck.
One guess is that net_rx_action spent more than 1 tick processing all the incoming packets for all interfaces it polled; I verified that my driver by itself does not spend that much. When this happens, net_rx_action exits after marking NETIF_RX_SOFTIRQ as pending. So one would expect it to be called again later, but my guess is that doesn't happen thereby resulting in a stoppage of incoming packets. Is that possible and, if so, what is the fix?
1814 static void net_rx_action(struct softirq_action *h)
1815 {
1816 struct softnet_data *queue = &__get_cpu_var(softnet_data);
1817 unsigned long start_time = jiffies;
1818 int budget = netdev_budget;
1819 void *have;
1820
1821 local_irq_disable();
1822
1823 while (!list_empty(&queue->poll_list)) {
1824 struct net_device *dev;
1825
1826 if (budget <= 0 || jiffies - start_time > 1)
1827 goto softnet_break;
1828
1829 local_irq_enable();
1830
1831 dev = list_entry(queue->poll_list.next,
1832 struct net_device, poll_list);
1833 have = netpoll_poll_lock(dev);
1834
1835 if (dev->quota <= 0 || dev->poll(dev, &budget)) {
1836 netpoll_poll_unlock(have);
1837 local_irq_disable();
1838 list_move_tail(&dev->poll_list, &queue->poll_list);
1839 if (dev->quota < 0)
1840 dev->quota += dev->weight;
1841 else
1842 dev->quota = dev->weight;
1843 } else {
1844 netpoll_poll_unlock(have);
1845 dev_put(dev);
1846 local_irq_disable();
1847 }
1848 }
1849 out:
1850 local_irq_enable();
1851 return;
1852
1853 softnet_break:
1854 __get_cpu_var(netdev_rx_stat).time_squeeze++;
1855 __raise_softirq_irqoff(NET_RX_SOFTIRQ);
1856 goto out;
1857 }
1858
I have run into this problem on four systems running RHEL5, SLES10 or SLES 11. The above describes what happens in RHEL5/SLES10. This is different in SLES11, wherein dev->poll has been replaced by netif_napi_add and the poll function returns done without quota/budget manipulation; yet, I run into the same behavior.
Any help appreciated! Thanks in advance!
Usha
___________________
Usha Srinivasan
Software Engineer
QLogic Corporation
780 5th Ave, Suite A
King of Prussia, PA 19406
(610) 233-4844
(610) 233-4777 (Fax)
(610) 233-4838 (Main Desk)
^ permalink raw reply
* Re: [patch] isdn: fix information leak
From: Changli Gao @ 2010-08-05 13:59 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Karsten Keil, netdev, kernel-janitors
In-Reply-To: <20100805134910.GJ9031@bicker>
On Thu, Aug 5, 2010 at 9:55 PM, Dan Carpenter <error27@gmail.com> wrote:
> First of all, I'm happy that you're reviewing these patches. A lot of
> learner, often untested, patches go through kernel-janitors and the
> entire purpose of the list is to check each other's work.
>
> Here is what I meant by an information leak. The original code did:
> spid = kmalloc(SCIOC_SPIDSIZE, GFP_KERNEL);
> ...
> strcpy(spid, rcvmsg->msg_data.byte_array);
> ...
> if (copy_to_user(data->dataptr, spid, SCIOC_SPIDSIZE)) {
>
> When you allocate memory with kmalloc() it doesn't clear that memory. It
> could have anything in there including passwords etc. If there is a
> short string in "rcvmsg->msg_data.byte_array" like "123" then the
> strcpy() puts that in the first 4 bytes of "spid", but the rest of the
> buffer is still full of passwords.
>
> The copy_to_user() sends it to the user, and normally the user only reads
> as far as the terminator, but if they read past that then they would see
> all the passwords etc that were saved there.
>
> This function is in the ioctl() probably only root has open() permission
> on the device so it's not a big deal because root can already find your
> password. But I didn't dig that far. It's just simpler to zero out the
> memory instead of worrying about if it really is exploitable or not.
>
> Also it's quite possible that the strcpy can't overflow. But it's weird
> for the code to leave space for the terminator and then not use it. In
> the end, I decided to do the cautious thing and be done with it.
>
Thanks, Dan. I got it.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [patch] isdn: fix information leak
From: Dan Carpenter @ 2010-08-05 13:55 UTC (permalink / raw)
To: Changli Gao; +Cc: Karsten Keil, netdev, kernel-janitors
In-Reply-To: <AANLkTikGNr8aVV7Rryq_dvSuYjUbEo_gRDLW3d49YwF-@mail.gmail.com>
First of all, I'm happy that you're reviewing these patches. A lot of
learner, often untested, patches go through kernel-janitors and the
entire purpose of the list is to check each other's work.
Here is what I meant by an information leak. The original code did:
spid = kmalloc(SCIOC_SPIDSIZE, GFP_KERNEL);
...
strcpy(spid, rcvmsg->msg_data.byte_array);
...
if (copy_to_user(data->dataptr, spid, SCIOC_SPIDSIZE)) {
When you allocate memory with kmalloc() it doesn't clear that memory. It
could have anything in there including passwords etc. If there is a
short string in "rcvmsg->msg_data.byte_array" like "123" then the
strcpy() puts that in the first 4 bytes of "spid", but the rest of the
buffer is still full of passwords.
The copy_to_user() sends it to the user, and normally the user only reads
as far as the terminator, but if they read past that then they would see
all the passwords etc that were saved there.
This function is in the ioctl() probably only root has open() permission
on the device so it's not a big deal because root can already find your
password. But I didn't dig that far. It's just simpler to zero out the
memory instead of worrying about if it really is exploitable or not.
Also it's quite possible that the strcpy can't overflow. But it's weird
for the code to leave space for the terminator and then not use it. In
the end, I decided to do the cautious thing and be done with it.
regards,
dan carpenter
^ 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