* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
From: Jason Wang @ 2013-01-10 14:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
In-Reply-To: <20130110113108.GF13451@redhat.com>
On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
> At the moment, we check owner when we enable queue in tun.
> This seems redundant and will break some valid uses
> where fd is passed around: I think TUNSETOWNER is there
> to prevent others from attaching to a persistent device not
> owned by them. Here the fd is already attached,
> enabling/disabling queue is more like read/write.
It also change the number of queues of the tuntap, maybe we should limit
this. Note that if management layer does not call TUNSETOWNER, the check
is just a nop. So if management layer want to limit the behavior, it's
its duty to do this correctly.
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> Note: this is unrelated to Stefan's bugfix.
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index fbd106e..78e3225 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
> tun = tfile->detached;
> if (!tun)
> ret = -EINVAL;
> - else if (tun_not_capable(tun))
> - ret = -EPERM;
> else
> ret = tun_attach(tun, file);
> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
> tun = rcu_dereference_protected(tfile->tun,
> lockdep_rtnl_is_held());
^ permalink raw reply
* Re: [PATCH] CDC_NCM adding support IFF_NOARP for buggy device
From: Oliver Neukum @ 2013-01-10 14:10 UTC (permalink / raw)
To: Wei Shuai; +Cc: gregkh, alexey.orishko, hans.petter.selasky, linux-usb, netdev
In-Reply-To: <1357826368-30543-1-git-send-email-cpuwolf@gmail.com>
On Thursday 10 January 2013 21:59:28 Wei Shuai wrote:
Hi,
I am afraid this is done in an unclean manner.
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 71b6e92..9903f79 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -545,6 +545,7 @@ EXPORT_SYMBOL_GPL(cdc_ncm_unbind);
> static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
> {
> int ret;
> + struct usb_device *udev = interface_to_usbdev(intf);
>
> /* The MBIM spec defines a NCM compatible default altsetting,
> * which we may have matched:
> @@ -572,6 +573,10 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
> cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
> return -ENODEV;
> #endif
> + /* this buggy device cannot do ARP */
> + if( (le16_to_cpu(udev->descriptor.idVendor) == 0xAAAA) ) {
> + dev->net->flags |= IFF_NOARP;
So either you do a check here ...
> + }
>
> /* NCM data altsetting is always 1 */
> ret = cdc_ncm_bind_common(dev, intf, 1);
> @@ -1186,6 +1191,16 @@ static const struct usb_device_id cdc_devs[] = {
> .driver_info = (unsigned long) &wwan_info,
> },
>
> + /* buggy device cannot send ARP */
> + { .match_flags = USB_DEVICE_ID_MATCH_INT_INFO
> + | USB_DEVICE_ID_MATCH_VENDOR,
> + .idVendor = 0xAAAA,
> + .bInterfaceClass = USB_CLASS_COMM,
> + .bInterfaceSubClass = USB_CDC_SUBCLASS_NCM,
> + .bInterfaceProtocol = USB_CDC_PROTO_NONE,
> + .driver_info = (unsigned long) &wwan_info,
... or you define a matching id here and give it a flag in driver_info
But not both.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
From: Michael S. Tsirkin @ 2013-01-10 14:19 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
In-Reply-To: <50EECB43.8020405@redhat.com>
On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
> > At the moment, we check owner when we enable queue in tun.
> > This seems redundant and will break some valid uses
> > where fd is passed around: I think TUNSETOWNER is there
> > to prevent others from attaching to a persistent device not
> > owned by them. Here the fd is already attached,
> > enabling/disabling queue is more like read/write.
>
> It also change the number of queues of the tuntap, maybe we should limit
> this.
Number of active queues? Why does it matter?
Max number of queues is already limited by SETIFF.
> Note that if management layer does not call TUNSETOWNER, the check
> is just a nop. So if management layer want to limit the behavior, it's
> its duty to do this correctly.
The point is that management limits tun to allow SETIFF from libvirt
only, then passes the fds to qemu.
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ---
> >
> > Note: this is unrelated to Stefan's bugfix.
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index fbd106e..78e3225 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
> > tun = tfile->detached;
> > if (!tun)
> > ret = -EINVAL;
> > - else if (tun_not_capable(tun))
> > - ret = -EPERM;
> > else
> > ret = tun_attach(tun, file);
> > } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
> > tun = rcu_dereference_protected(tfile->tun,
> > lockdep_rtnl_is_held());
>
^ permalink raw reply
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
From: Jason Wang @ 2013-01-10 14:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
In-Reply-To: <20130110141939.GA30557@redhat.com>
On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
>> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
>>> At the moment, we check owner when we enable queue in tun.
>>> This seems redundant and will break some valid uses
>>> where fd is passed around: I think TUNSETOWNER is there
>>> to prevent others from attaching to a persistent device not
>>> owned by them. Here the fd is already attached,
>>> enabling/disabling queue is more like read/write.
>> It also change the number of queues of the tuntap, maybe we should limit
>> this.
> Number of active queues? Why does it matter?
> Max number of queues is already limited by SETIFF.
Yes the number of active(real) queues in the kernel net device and this
changing may introduce other events such uevent. With this patch, even
if a owner is set for tap, every user could change the number of real
queues which I don't think is not expected. Without this patch, we can
limit a user that just do read and write.
>
>> Note that if management layer does not call TUNSETOWNER, the check
>> is just a nop. So if management layer want to limit the behavior, it's
>> its duty to do this correctly.
> The point is that management limits tun to allow SETIFF from libvirt
> only, then passes the fds to qemu.
Yes, but looks like libvirt does not call TUNSETOWNER before passing it
to qemu, so we're ok even without this patch. And if libvirt want to do
this, it can just call TUNSETOWNER to the user of qemu.
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> ---
>>>
>>> Note: this is unrelated to Stefan's bugfix.
>>>
>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>> index fbd106e..78e3225 100644
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
>>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
>>> tun = tfile->detached;
>>> if (!tun)
>>> ret = -EINVAL;
>>> - else if (tun_not_capable(tun))
>>> - ret = -EPERM;
>>> else
>>> ret = tun_attach(tun, file);
>>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
>>> tun = rcu_dereference_protected(tfile->tun,
>>> lockdep_rtnl_is_held());
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
From: Michael S. Tsirkin @ 2013-01-10 14:41 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
In-Reply-To: <50EECFC8.9020402@redhat.com>
On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote:
> On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote:
> > On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
> >> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
> >>> At the moment, we check owner when we enable queue in tun.
> >>> This seems redundant and will break some valid uses
> >>> where fd is passed around: I think TUNSETOWNER is there
> >>> to prevent others from attaching to a persistent device not
> >>> owned by them. Here the fd is already attached,
> >>> enabling/disabling queue is more like read/write.
> >> It also change the number of queues of the tuntap, maybe we should limit
> >> this.
> > Number of active queues? Why does it matter?
> > Max number of queues is already limited by SETIFF.
>
> Yes the number of active(real) queues in the kernel net device and this
> changing may introduce other events such uevent.
How can it trigger a uevent?
> With this patch, even
> if a owner is set for tap, every user could change the number of real
> queues which I don't think is not expected. Without this patch, we can
> limit a user that just do read and write.
In the end if you want very fine tuned security policy you have to
use an LSM.
Here we are talking about the expected usage without an LSM.
There, enabling/disabling queues is just an optimization:
if an application wants to process data from a single thread
it's better off getting it through a single fd.
Having to channel threading changes through a priveledged
proxy would be very awkward.
> >
> >> Note that if management layer does not call TUNSETOWNER, the check
> >> is just a nop. So if management layer want to limit the behavior, it's
> >> its duty to do this correctly.
> > The point is that management limits tun to allow SETIFF from libvirt
> > only, then passes the fds to qemu.
>
> Yes, but looks like libvirt does not call TUNSETOWNER before passing it
> to qemu, so we're ok even without this patch. And if libvirt want to do
> this, it can just call TUNSETOWNER to the user of qemu.
No, that would allow qemu to do SETIFF which we don't want.
> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>
> >>> ---
> >>>
> >>> Note: this is unrelated to Stefan's bugfix.
> >>>
> >>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >>> index fbd106e..78e3225 100644
> >>> --- a/drivers/net/tun.c
> >>> +++ b/drivers/net/tun.c
> >>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
> >>> tun = tfile->detached;
> >>> if (!tun)
> >>> ret = -EINVAL;
> >>> - else if (tun_not_capable(tun))
> >>> - ret = -EPERM;
> >>> else
> >>> ret = tun_attach(tun, file);
> >>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
> >>> tun = rcu_dereference_protected(tfile->tun,
> >>> lockdep_rtnl_is_held());
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] bfin_mac: Restore hardware time-stamping dependency on BF518
From: Lars-Peter Clausen @ 2013-01-10 14:42 UTC (permalink / raw)
To: David S. Miller
Cc: uclinux-dist-devel-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b,
Richard Cochran, netdev-u79uwXL29TY76Z2rM5mHXA
Commit 70ac618c07 ("ptp: fixup Kconfig for two PHC drivers.") removed all
dependencies for the blackfin hardware time-stamping Kconfig entry. Hardware
time-stamping is only available on BF518 though. Since the Kconfig entry is
'default y', just updateing your kernel source and running `make defconfig` will
result in the the following build errors:
drivers/net/ethernet/adi/bfin_mac.c:694: error: implicit declaration of function ‘bfin_read_EMAC_PTP_CTL’
drivers/net/ethernet/adi/bfin_mac.c:702: error: implicit declaration of function ‘bfin_write_EMAC_PTP_FV3’
drivers/net/ethernet/adi/bfin_mac.c:712: error: implicit declaration of function ‘bfin_write_EMAC_PTP_CTL’
drivers/net/ethernet/adi/bfin_mac.c:717: error: implicit declaration of function ‘bfin_write_EMAC_PTP_FOFF’
...
This patch adds back the dependency on BF518, and since it does not make sense
to expose this config option when the blackfin MAC driver is not enabled also
restore the dependency on BFIN_MAC.
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
drivers/net/ethernet/adi/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/adi/Kconfig b/drivers/net/ethernet/adi/Kconfig
index e49c0ef..a948160 100644
--- a/drivers/net/ethernet/adi/Kconfig
+++ b/drivers/net/ethernet/adi/Kconfig
@@ -61,6 +61,7 @@ config BFIN_RX_DESC_NUM
config BFIN_MAC_USE_HWSTAMP
bool "Use IEEE 1588 hwstamp"
+ depends on BFIN_MAC && BF518
select PTP_1588_CLOCK
default y
---help---
--
1.8.0
_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel@blackfin.uclinux.org
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
^ permalink raw reply related
* Re: [PATCH 2/2] Fix phy_attach - forward dev_flags for phy_attach
From: Florian Fainelli @ 2013-01-10 14:40 UTC (permalink / raw)
To: Kosta Zertsekel
Cc: netdev, zertsekel, andrew, benavi, linux-arm-kernel, alior
In-Reply-To: <1357819234-27752-3-git-send-email-konszert@marvell.com>
Le 01/10/13 13:00, Kosta Zertsekel a écrit :
> Change-Id: Ie3191f95c36eada6d0c673460de5393641128182
> ---
> drivers/net/ethernet/marvell/pxa168_eth.c | 2 +-
> net/dsa/slave.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
I think that you should have actually two patches, one for pxa168_eth
and one for net/dsa/slave.c.
Also, please prefix your patches with what was usually used on these
files before: "pxa168_eth:" and "net: dsa:" respectively.
By the way, most, if not all of the phy_connect() users in
drivers/net/ethernet/ also do not ensure they pass the phy device flags,
so you might want to fix this globally and not just for Marvell driver.
Thanks.
--
Florian
^ permalink raw reply
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
From: Jason Wang @ 2013-01-10 14:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
In-Reply-To: <20130110144138.GA30731@redhat.com>
On 01/10/2013 10:41 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote:
>> On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote:
>>> On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
>>>> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
>>>>> At the moment, we check owner when we enable queue in tun.
>>>>> This seems redundant and will break some valid uses
>>>>> where fd is passed around: I think TUNSETOWNER is there
>>>>> to prevent others from attaching to a persistent device not
>>>>> owned by them. Here the fd is already attached,
>>>>> enabling/disabling queue is more like read/write.
>>>> It also change the number of queues of the tuntap, maybe we should limit
>>>> this.
>>> Number of active queues? Why does it matter?
>>> Max number of queues is already limited by SETIFF.
>> Yes the number of active(real) queues in the kernel net device and this
>> changing may introduce other events such uevent.
> How can it trigger a uevent?
netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which
may trigger an uevent.
>
>> With this patch, even
>> if a owner is set for tap, every user could change the number of real
>> queues which I don't think is not expected. Without this patch, we can
>> limit a user that just do read and write.
> In the end if you want very fine tuned security policy you have to
> use an LSM.
>
> Here we are talking about the expected usage without an LSM.
> There, enabling/disabling queues is just an optimization:
> if an application wants to process data from a single thread
> it's better off getting it through a single fd.
> Having to channel threading changes through a priveledged
> proxy would be very awkward.
Yes, but we have something similar like bridge-helper in qemu which
create devices through a privileged proxy.
>>>> Note that if management layer does not call TUNSETOWNER, the check
>>>> is just a nop. So if management layer want to limit the behavior, it's
>>>> its duty to do this correctly.
>>> The point is that management limits tun to allow SETIFF from libvirt
>>> only, then passes the fds to qemu.
>> Yes, but looks like libvirt does not call TUNSETOWNER before passing it
>> to qemu, so we're ok even without this patch. And if libvirt want to do
>> this, it can just call TUNSETOWNER to the user of qemu.
> No, that would allow qemu to do SETIFF which we don't want.
True, I was wrong.
>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Note: this is unrelated to Stefan's bugfix.
>>>>>
>>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>>>> index fbd106e..78e3225 100644
>>>>> --- a/drivers/net/tun.c
>>>>> +++ b/drivers/net/tun.c
>>>>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
>>>>> tun = tfile->detached;
>>>>> if (!tun)
>>>>> ret = -EINVAL;
>>>>> - else if (tun_not_capable(tun))
>>>>> - ret = -EPERM;
>>>>> else
>>>>> ret = tun_attach(tun, file);
>>>>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
>>>>> tun = rcu_dereference_protected(tfile->tun,
>>>>> lockdep_rtnl_is_held());
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
From: Michael S. Tsirkin @ 2013-01-10 15:10 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
In-Reply-To: <50EED495.4040405@redhat.com>
On Thu, Jan 10, 2013 at 10:47:49PM +0800, Jason Wang wrote:
> On 01/10/2013 10:41 PM, Michael S. Tsirkin wrote:
> > On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote:
> >> On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote:
> >>> On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
> >>>> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
> >>>>> At the moment, we check owner when we enable queue in tun.
> >>>>> This seems redundant and will break some valid uses
> >>>>> where fd is passed around: I think TUNSETOWNER is there
> >>>>> to prevent others from attaching to a persistent device not
> >>>>> owned by them. Here the fd is already attached,
> >>>>> enabling/disabling queue is more like read/write.
> >>>> It also change the number of queues of the tuntap, maybe we should limit
> >>>> this.
> >>> Number of active queues? Why does it matter?
> >>> Max number of queues is already limited by SETIFF.
> >> Yes the number of active(real) queues in the kernel net device and this
> >> changing may introduce other events such uevent.
> > How can it trigger a uevent?
>
> netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which
> may trigger an uevent.
Look SETOWNER is a tool intended mostly for persistent taps,
where you give a specific user the rights to attach to
specific taps but not others.
The uevent issue is preventing a DOS by a uevent flood?
Then it applies to persistent and non persistent as one.
So if one cares about this one should use an LSM
or we can add a separate capability to limit this if we
care enough.
> >
> >> With this patch, even
> >> if a owner is set for tap, every user could change the number of real
> >> queues which I don't think is not expected. Without this patch, we can
> >> limit a user that just do read and write.
> > In the end if you want very fine tuned security policy you have to
> > use an LSM.
> >
> > Here we are talking about the expected usage without an LSM.
> > There, enabling/disabling queues is just an optimization:
> > if an application wants to process data from a single thread
> > it's better off getting it through a single fd.
> > Having to channel threading changes through a priveledged
> > proxy would be very awkward.
>
> Yes, but we have something similar like bridge-helper in qemu which
> create devices through a privileged proxy.
This only happens on startup. Threading changes can happen
at any time.
> >>>> Note that if management layer does not call TUNSETOWNER, the check
> >>>> is just a nop. So if management layer want to limit the behavior, it's
> >>>> its duty to do this correctly.
> >>> The point is that management limits tun to allow SETIFF from libvirt
> >>> only, then passes the fds to qemu.
> >> Yes, but looks like libvirt does not call TUNSETOWNER before passing it
> >> to qemu, so we're ok even without this patch. And if libvirt want to do
> >> this, it can just call TUNSETOWNER to the user of qemu.
> > No, that would allow qemu to do SETIFF which we don't want.
>
> True, I was wrong.
> >
> >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>>
> >>>>> ---
> >>>>>
> >>>>> Note: this is unrelated to Stefan's bugfix.
> >>>>>
> >>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >>>>> index fbd106e..78e3225 100644
> >>>>> --- a/drivers/net/tun.c
> >>>>> +++ b/drivers/net/tun.c
> >>>>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
> >>>>> tun = tfile->detached;
> >>>>> if (!tun)
> >>>>> ret = -EINVAL;
> >>>>> - else if (tun_not_capable(tun))
> >>>>> - ret = -EPERM;
> >>>>> else
> >>>>> ret = tun_attach(tun, file);
> >>>>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
> >>>>> tun = rcu_dereference_protected(tfile->tun,
> >>>>> lockdep_rtnl_is_held());
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe netdev" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: recvmmsg() timeout behavior strangeness
From: Chris Friesen @ 2013-01-10 15:24 UTC (permalink / raw)
To: Steven Whitehouse
Cc: Michael Kerrisk, Arnaldo Carvalho de Melo, Caitlin Bestler,
David Miller, netdev, Chris Van Hoof, Clark Williams, Neil Horman,
Arnaldo Carvalho de Melo, Andrew Grover, Elie De Brauwer,
linux-man, Rémi Denis-Courmont
In-Reply-To: <1357812249.2760.6.camel@menhir>
On 01/10/2013 04:04 AM, Steven Whitehouse wrote:
> Which timeout are we talking about? I've been copied into the thread
> without seeing the start of it.
The discussion is about the timeout parameter for the recvmmsg() call.
Currently in the recvmmsg() code it only checks the timeout after
__sys_recvmsg() returns, so if __sys_recvmsg() blocks forever we could
end up essentially ignoring the timeout.
> If this is the rcvtimeo then afaik this is supposed to be the max time
> that the call waits for data, but is overridden by MSG_DONTWAIT, for
> example, on a per call basis. I'd assume that recvmmsg should work
> exactly like recvmsg in this case unless there is a good reason for it
> to differ,
recvmsg() doesn't have a timeout parameter, so it uses SO_RCVTIMEO.
recvmmsg() has an explicit timeout parameter but it doesn't look like it
works properly and the documentation doesn't mention how it is supposed
to interact with SO_RCVTIMEO.
Chris
^ permalink raw reply
* Re: [PATCH] tun: avoid owner checks on IFF_ATTACH_QUEUE
From: Jason Wang @ 2013-01-10 15:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, David S. Miller, netdev, linux-kernel,
Paul Moore
In-Reply-To: <20130110151037.GD30731@redhat.com>
On 01/10/2013 11:10 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 10, 2013 at 10:47:49PM +0800, Jason Wang wrote:
>> On 01/10/2013 10:41 PM, Michael S. Tsirkin wrote:
>>> On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote:
>>>> On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote:
>>>>> On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote:
>>>>>> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote:
>>>>>>> At the moment, we check owner when we enable queue in tun.
>>>>>>> This seems redundant and will break some valid uses
>>>>>>> where fd is passed around: I think TUNSETOWNER is there
>>>>>>> to prevent others from attaching to a persistent device not
>>>>>>> owned by them. Here the fd is already attached,
>>>>>>> enabling/disabling queue is more like read/write.
>>>>>> It also change the number of queues of the tuntap, maybe we should limit
>>>>>> this.
>>>>> Number of active queues? Why does it matter?
>>>>> Max number of queues is already limited by SETIFF.
>>>> Yes the number of active(real) queues in the kernel net device and this
>>>> changing may introduce other events such uevent.
>>> How can it trigger a uevent?
>> netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which
>> may trigger an uevent.
> Look SETOWNER is a tool intended mostly for persistent taps,
> where you give a specific user the rights to attach to
> specific taps but not others.
True.
> The uevent issue is preventing a DOS by a uevent flood?
> Then it applies to persistent and non persistent as one.
> So if one cares about this one should use an LSM
> or we can add a separate capability to limit this if we
> care enough.
Ok.
>>>> With this patch, even
>>>> if a owner is set for tap, every user could change the number of real
>>>> queues which I don't think is not expected. Without this patch, we can
>>>> limit a user that just do read and write.
>>> In the end if you want very fine tuned security policy you have to
>>> use an LSM.
>>>
>>> Here we are talking about the expected usage without an LSM.
>>> There, enabling/disabling queues is just an optimization:
>>> if an application wants to process data from a single thread
>>> it's better off getting it through a single fd.
>>> Having to channel threading changes through a priveledged
>>> proxy would be very awkward.
>> Yes, but we have something similar like bridge-helper in qemu which
>> create devices through a privileged proxy.
> This only happens on startup. Threading changes can happen
> at any time.
Yes. So no objection from my side. Thanks for the explanation.
>>>>>> Note that if management layer does not call TUNSETOWNER, the check
>>>>>> is just a nop. So if management layer want to limit the behavior, it's
>>>>>> its duty to do this correctly.
>>>>> The point is that management limits tun to allow SETIFF from libvirt
>>>>> only, then passes the fds to qemu.
>>>> Yes, but looks like libvirt does not call TUNSETOWNER before passing it
>>>> to qemu, so we're ok even without this patch. And if libvirt want to do
>>>> this, it can just call TUNSETOWNER to the user of qemu.
>>> No, that would allow qemu to do SETIFF which we don't want.
>> True, I was wrong.
>>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> Note: this is unrelated to Stefan's bugfix.
>>>>>>>
>>>>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>>>>>> index fbd106e..78e3225 100644
>>>>>>> --- a/drivers/net/tun.c
>>>>>>> +++ b/drivers/net/tun.c
>>>>>>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
>>>>>>> tun = tfile->detached;
>>>>>>> if (!tun)
>>>>>>> ret = -EINVAL;
>>>>>>> - else if (tun_not_capable(tun))
>>>>>>> - ret = -EPERM;
>>>>>>> else
>>>>>>> ret = tun_attach(tun, file);
>>>>>>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
>>>>>>> tun = rcu_dereference_protected(tfile->tun,
>>>>>>> lockdep_rtnl_is_held());
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: tainted warnings with tcp splicing in 3.7.1
From: Eric Dumazet @ 2013-01-10 15:29 UTC (permalink / raw)
To: Willy Tarreau; +Cc: Christian Becker, David Miller, netdev@vger.kernel.org
In-Reply-To: <20130110072136.GK9943@1wt.eu>
On Thu, 2013-01-10 at 08:21 +0100, Willy Tarreau wrote:
> FWIW, I tested your patch here and did not notice any regression
> compared to last week-end tests, at various MTU size combinations.
>
Thanks Willy
Tested-by: Willy Tarreau <w@1wt.eu>
I believe I need to fix net-next,
commit 9ca1b22d6d228177e6f929f6818a1cd3d5e30c4a
(net: splice: avoid high order page splitting)
missed the loopback case : the skb->head might need several
linear_to_page() calls.
I'll send a patch after full testing.
^ permalink raw reply
* [PATCH net 0/2] bnx2x: bug fix patch series
From: Yuval Mintz @ 2013-01-10 14:53 UTC (permalink / raw)
To: davem, netdev; +Cc: eilong, ariele, Yuval Mintz
Hi Dave,
This patch series contains 2 bug fixes:
1. Failed memory allocation during Tx queue allocation can
later cause the driver to dereference incorrect pointers,
causing a panic.
2. During probe, management traffic is stopped and resumes
only after some of its functions gets loaded.
Please consider applying this patch-series to 'net'.
Thanks,
Yuval Mintz
^ permalink raw reply
* [PATCH net 1/2] bnx2x: Fix fastpath structures when memory allocation fails
From: Yuval Mintz @ 2013-01-10 14:53 UTC (permalink / raw)
To: davem, netdev; +Cc: eilong, ariele, Yuval Mintz
In-Reply-To: <1357829620-17890-1-git-send-email-yuvalmin@broadcom.com>
When allocating Tx queues, if for some reason
(e.g., lack of memory) allocation fails, driver will incorrectly
calculate the pointers of the various queues.
This patch repositions all pointers in such a case to point at
sequential structures in memory, allowing the bnx2x macros to
be used correctly when accessing them.
Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
Signed-off-by: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 30 +++++++++++++++++++++-
1 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 01588b6..f771ddf 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -80,12 +80,37 @@ static inline void bnx2x_move_fp(struct bnx2x *bp, int from, int to)
new_txdata_index = new_max_eth_txqs + FCOE_TXQ_IDX_OFFSET;
}
- memcpy(&bp->bnx2x_txq[old_txdata_index],
- &bp->bnx2x_txq[new_txdata_index],
+ memcpy(&bp->bnx2x_txq[new_txdata_index],
+ &bp->bnx2x_txq[old_txdata_index],
sizeof(struct bnx2x_fp_txdata));
to_fp->txdata_ptr[0] = &bp->bnx2x_txq[new_txdata_index];
}
+/**
+ * bnx2x_shrink_eth_fp - guarantees fastpath structures stay intact
+ *
+ * @bp: driver handle
+ * @delta: number of eth queues which were not allocated
+ */
+static void bnx2x_shrink_eth_fp(struct bnx2x *bp, int delta)
+{
+ int i, cos, old_eth_num = BNX2X_NUM_ETH_QUEUES(bp);
+
+ /* Queue pointer cannot be re-set on an fp-basis, as moving pointer
+ * backward along the array could cause memory to be overriden
+ */
+ for (cos = 1; cos < bp->max_cos; cos++) {
+ for (i = 0; i < old_eth_num - delta; i++) {
+ struct bnx2x_fastpath *fp = &bp->fp[i];
+ int new_idx = cos * (old_eth_num - delta) + i;
+
+ memcpy(&bp->bnx2x_txq[new_idx], fp->txdata_ptr[cos],
+ sizeof(struct bnx2x_fp_txdata));
+ fp->txdata_ptr[cos] = &bp->bnx2x_txq[new_idx];
+ }
+ }
+}
+
int load_count[2][3] = { {0} }; /* per-path: 0-common, 1-port0, 2-port1 */
/* free skb in the packet ring at pos idx
@@ -3863,6 +3888,7 @@ int bnx2x_alloc_fp_mem(struct bnx2x *bp)
int delta = BNX2X_NUM_ETH_QUEUES(bp) - i;
WARN_ON(delta < 0);
+ bnx2x_shrink_eth_fp(bp, delta);
if (CNIC_SUPPORT(bp))
/* move non eth FPs next to last eth FP
* must be done in that order
--
1.7.1
^ permalink raw reply related
* [PATCH net 2/2] bnx2x: Allow management traffic after boot from SAN
From: Yuval Mintz @ 2013-01-10 14:53 UTC (permalink / raw)
To: davem, netdev; +Cc: eilong, ariele, Barak Witkowski, Yuval Mintz
In-Reply-To: <1357829620-17890-1-git-send-email-yuvalmin@broadcom.com>
From: Barak Witkowski <barak@broadcom.com>
As part of the previous driver unload flow, whenever bnx2x is
loaded after the UNDI driver it closes all Rx traffic.
However, this leads to management traffic also being stopped until
the network interface associated with one of its functions gets loaded.
To remedy this, management traffic is re-opened once the 'cleaning'
after the previous driver ends.
Signed-off-by: Barak Witkowski <barak@broadcom.com>
Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
Signed-off-by: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 60 +++++++++++++++++++---
1 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 940ef85..5523da3 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -127,6 +127,17 @@ MODULE_PARM_DESC(debug, " Default debug msglevel");
struct workqueue_struct *bnx2x_wq;
+struct bnx2x_mac_vals {
+ u32 xmac_addr;
+ u32 xmac_val;
+ u32 emac_addr;
+ u32 emac_val;
+ u32 umac_addr;
+ u32 umac_val;
+ u32 bmac_addr;
+ u32 bmac_val[2];
+};
+
enum bnx2x_board_type {
BCM57710 = 0,
BCM57711,
@@ -9420,12 +9431,19 @@ static inline void bnx2x_undi_int_disable(struct bnx2x *bp)
bnx2x_undi_int_disable_e1h(bp);
}
-static void bnx2x_prev_unload_close_mac(struct bnx2x *bp)
+static void bnx2x_prev_unload_close_mac(struct bnx2x *bp,
+ struct bnx2x_mac_vals *vals)
{
u32 val, base_addr, offset, mask, reset_reg;
bool mac_stopped = false;
u8 port = BP_PORT(bp);
+ /* reset addresses as they also mark which values were changed */
+ vals->bmac_addr = 0;
+ vals->umac_addr = 0;
+ vals->xmac_addr = 0;
+ vals->emac_addr = 0;
+
reset_reg = REG_RD(bp, MISC_REG_RESET_REG_2);
if (!CHIP_IS_E3(bp)) {
@@ -9447,14 +9465,18 @@ static void bnx2x_prev_unload_close_mac(struct bnx2x *bp)
*/
wb_data[0] = REG_RD(bp, base_addr + offset);
wb_data[1] = REG_RD(bp, base_addr + offset + 0x4);
+ vals->bmac_addr = base_addr + offset;
+ vals->bmac_val[0] = wb_data[0];
+ vals->bmac_val[1] = wb_data[1];
wb_data[0] &= ~BMAC_CONTROL_RX_ENABLE;
- REG_WR(bp, base_addr + offset, wb_data[0]);
- REG_WR(bp, base_addr + offset + 0x4, wb_data[1]);
+ REG_WR(bp, vals->bmac_addr, wb_data[0]);
+ REG_WR(bp, vals->bmac_addr + 0x4, wb_data[1]);
}
BNX2X_DEV_INFO("Disable emac Rx\n");
- REG_WR(bp, NIG_REG_NIG_EMAC0_EN + BP_PORT(bp)*4, 0);
-
+ vals->emac_addr = NIG_REG_NIG_EMAC0_EN + BP_PORT(bp)*4;
+ vals->emac_val = REG_RD(bp, vals->emac_addr);
+ REG_WR(bp, vals->emac_addr, 0);
mac_stopped = true;
} else {
if (reset_reg & MISC_REGISTERS_RESET_REG_2_XMAC) {
@@ -9465,14 +9487,18 @@ static void bnx2x_prev_unload_close_mac(struct bnx2x *bp)
val & ~(1 << 1));
REG_WR(bp, base_addr + XMAC_REG_PFC_CTRL_HI,
val | (1 << 1));
- REG_WR(bp, base_addr + XMAC_REG_CTRL, 0);
+ vals->xmac_addr = base_addr + XMAC_REG_CTRL;
+ vals->xmac_val = REG_RD(bp, vals->xmac_addr);
+ REG_WR(bp, vals->xmac_addr, 0);
mac_stopped = true;
}
mask = MISC_REGISTERS_RESET_REG_2_UMAC0 << port;
if (mask & reset_reg) {
BNX2X_DEV_INFO("Disable umac Rx\n");
base_addr = BP_PORT(bp) ? GRCBASE_UMAC1 : GRCBASE_UMAC0;
- REG_WR(bp, base_addr + UMAC_REG_COMMAND_CONFIG, 0);
+ vals->umac_addr = base_addr + UMAC_REG_COMMAND_CONFIG;
+ vals->umac_val = REG_RD(bp, vals->umac_addr);
+ REG_WR(bp, vals->umac_addr, 0);
mac_stopped = true;
}
}
@@ -9664,12 +9690,16 @@ static int bnx2x_prev_unload_common(struct bnx2x *bp)
{
u32 reset_reg, tmp_reg = 0, rc;
bool prev_undi = false;
+ struct bnx2x_mac_vals mac_vals;
+
/* It is possible a previous function received 'common' answer,
* but hasn't loaded yet, therefore creating a scenario of
* multiple functions receiving 'common' on the same path.
*/
BNX2X_DEV_INFO("Common unload Flow\n");
+ memset(&mac_vals, 0, sizeof(mac_vals));
+
if (bnx2x_prev_is_path_marked(bp))
return bnx2x_prev_mcp_done(bp);
@@ -9680,7 +9710,10 @@ static int bnx2x_prev_unload_common(struct bnx2x *bp)
u32 timer_count = 1000;
/* Close the MAC Rx to prevent BRB from filling up */
- bnx2x_prev_unload_close_mac(bp);
+ bnx2x_prev_unload_close_mac(bp, &mac_vals);
+
+ /* close LLH filters towards the BRB */
+ bnx2x_set_rx_filter(&bp->link_params, 0);
/* Check if the UNDI driver was previously loaded
* UNDI driver initializes CID offset for normal bell to 0x7
@@ -9727,6 +9760,17 @@ static int bnx2x_prev_unload_common(struct bnx2x *bp)
/* No packets are in the pipeline, path is ready for reset */
bnx2x_reset_common(bp);
+ if (mac_vals.xmac_addr)
+ REG_WR(bp, mac_vals.xmac_addr, mac_vals.xmac_val);
+ if (mac_vals.umac_addr)
+ REG_WR(bp, mac_vals.umac_addr, mac_vals.umac_val);
+ if (mac_vals.emac_addr)
+ REG_WR(bp, mac_vals.emac_addr, mac_vals.emac_val);
+ if (mac_vals.bmac_addr) {
+ REG_WR(bp, mac_vals.bmac_addr, mac_vals.bmac_val[0]);
+ REG_WR(bp, mac_vals.bmac_addr + 4, mac_vals.bmac_val[1]);
+ }
+
rc = bnx2x_prev_mark_path(bp, prev_undi);
if (rc) {
bnx2x_prev_mcp_done(bp);
--
1.7.1
^ permalink raw reply related
* RE: [PATCH 2/2] Fix phy_attach - forward dev_flags for phy_attach
From: Kosta Zertsekel @ 2013-01-10 15:57 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev@vger.kernel.org, zertsekel@gmail.com, andrew@lunn.ch,
Eran Ben-Avi, linux-arm-kernel@lists.infradead.org, Lior Amsalem
In-Reply-To: <50EED2E4.80008@openwrt.org>
> By the way, most, if not all of the phy_connect() users in drivers/net/ethernet/ also do not ensure they pass the phy device flags, so you might
want to fix this globally and not just for Marvell driver.
Indeed, phy_connect() mostly just pass zero intead of phy_dev->dev_flags.
But, I think, the guy that calls phy_connect() in its driver should know what he does, and,
probably, we should rely on him knowing his stuff.
The only evidence of the bug is when phy_dev->dev_flags was actually changed by PHY fixup callback
(see dns323-setup.c for example) and was *not* propagated to phy_connect() or phy_attach() as in pxa168_eth.c phy_init().
The code conforming to the former should be fixed IMHO.
Thanks,
--- KostaZ
^ permalink raw reply
* Re: tainted warnings with tcp splicing in 3.7.1
From: Eric Dumazet @ 2013-01-10 16:20 UTC (permalink / raw)
To: Willy Tarreau, Rick Jones
Cc: Christian Becker, David Miller, netdev@vger.kernel.org
In-Reply-To: <1357831787.27446.2168.camel@edumazet-glaptop>
On Thu, 2013-01-10 at 07:29 -0800, Eric Dumazet wrote:
> On Thu, 2013-01-10 at 08:21 +0100, Willy Tarreau wrote:
>
> > FWIW, I tested your patch here and did not notice any regression
> > compared to last week-end tests, at various MTU size combinations.
> >
>
> Thanks Willy
I also want to thanks Rick, as the latest netperf has splice() support.
Thanks Rick !
^ permalink raw reply
* Re: [PATCH] CDC_NCM adding support IFF_NOARP for buggy device
From: Sergei Shtylyov @ 2013-01-10 17:18 UTC (permalink / raw)
To: Wei Shuai; +Cc: gregkh, alexey.orishko, hans.petter.selasky, linux-usb, netdev
In-Reply-To: <1357826368-30543-1-git-send-email-cpuwolf@gmail.com>
Hello.
On 01/10/2013 04:59 PM, Wei Shuai wrote:
> Signed-off-by: Wei Shuai <cpuwolf@gmail.com>
> ---
> drivers/net/usb/cdc_ncm.c | 15 +++++++++++++++
> 1 files changed, 15 insertions(+), 0 deletions(-)
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 71b6e92..9903f79 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
[...]
> @@ -572,6 +573,10 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
> cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
> return -ENODEV;
> #endif
> + /* this buggy device cannot do ARP */
> + if( (le16_to_cpu(udev->descriptor.idVendor) == 0xAAAA) ) {
Please run the patch thru scripts/checkpatch.pl and resubmit.
WBR, Sergei
^ permalink raw reply
* Re: [PATCH net-next] team: add ethtool support
From: Ben Hutchings @ 2013-01-10 16:27 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Flavio Leitner, Stephen Hemminger, netdev, Jiri Pirko
In-Reply-To: <1356834716.21409.6258.camel@edumazet-glaptop>
On Sat, 2012-12-29 at 18:31 -0800, Eric Dumazet wrote:
> On Sat, 2012-12-29 at 23:44 -0200, Flavio Leitner wrote:
>
> > Speaking as a support engineer, it's a lot easier to grab ethtool -S and
> > see everything than grab two or more outputs.
> >
>
> I agree its very convenient.
>
> I have a patch to add GRO statistics at the core layer, in the ethtool
> -S stats.
>
> I was about to ask netdev guys what they think of this idea ?
[...]
I would like to make these statistics available *but* I would prefer to
see a plan for properly integrating the basic statistics
(net_device_stats/rtnl_link_stats64), additional core statistics (such
as these) and driver-specific statistics.
It's annoying that users have to use different tools to get these, and
adding core statistics to ethtool (currently driver-specific) is just
going to be more confusing.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] bfin_mac: Restore hardware time-stamping dependency on BF518
From: Richard Cochran @ 2013-01-10 16:40 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: David S. Miller, uclinux-dist-devel, netdev
In-Reply-To: <1357828948-22682-1-git-send-email-lars@metafoo.de>
On Thu, Jan 10, 2013 at 03:42:28PM +0100, Lars-Peter Clausen wrote:
>
> This patch adds back the dependency on BF518, and since it does not make sense
> to expose this config option when the blackfin MAC driver is not enabled also
> restore the dependency on BFIN_MAC.
Thanks a lot for fixing this.
Acked-by: Richard Cochran <richardcochran@gmail.com>
^ permalink raw reply
* Re: [PATCH 01/19] netfilter: move nf_conntrack initialize out of pernet operations
From: Pablo Neira Ayuso @ 2013-01-10 16:41 UTC (permalink / raw)
To: Gao feng; +Cc: netfilter-devel, netdev, canqunzhang, kaber, ebiederm
In-Reply-To: <1356662206-2260-1-git-send-email-gaofeng@cn.fujitsu.com>
[-- Attachment #1: Type: text/plain, Size: 937 bytes --]
Hi Gao,
On Fri, Dec 28, 2012 at 10:36:27AM +0800, Gao feng wrote:
> canqun zhang reported a panic BUG,kernel may panic when
> unloading nf_conntrack module.
>
> It's because we reset nf_ct_destroy to NULL when we deal
> with init_net,it's too early.Some packets belongs to other
> netns still refers to the conntrack.when these packets need
> to be freed, kfree_skb will call nf_ct_destroy which is
> NULL.
>
> fix this bug by moving the nf_conntrack initialize and cleanup
> codes out of the pernet operations,this job should be done
> in module_init/exit.We can't use init_net to identify if
> it's the right time.
First off, thanks for looking into this.
I want to get this fix into 3.8 and -stable but this patch includes a
rework whose scope is net-next (upcoming 3.9).
The attached patch aims to fix the issue according to your patch
description. Once this is in, we can revisit your code refactoring
proposal.
Let me know.
[-- Attachment #2: 0001-netfilter-nf_conntrack-fix-BUG_ON-while-removing-nf_.patch --]
[-- Type: text/x-diff, Size: 2758 bytes --]
>From a211bd666fbfe17ae7171a50ad92fedc7b9e19fa Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 10 Jan 2013 16:12:01 +0100
Subject: [PATCH] netfilter: nf_conntrack: fix BUG_ON while removing
nf_conntrack with netns
canqun zhang reported that we're hitting BUG_ON in the
nf_conntrack_destroy path when calling kfree_skb while
rmmod'ing the nf_conntrack module.
Currently, the nf_ct_destroy hook is being set to NULL in the
destroy path of conntrack.init_net. However, this is a problem
since init_net may be destroyed before any other existing netns
(we cannot assume any specific ordering while releasing existing
netns according to what I read in recent emails).
Thanks to Gao feng for initial patch to address this issue.
Reported-by: canqun zhang <canqunzhang@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_conntrack_core.h | 2 ++
net/netfilter/nf_conntrack_core.c | 9 +++++----
net/netfilter/nf_conntrack_standalone.c | 1 +
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
index d8f5b9f..e98aeb3 100644
--- a/include/net/netfilter/nf_conntrack_core.h
+++ b/include/net/netfilter/nf_conntrack_core.h
@@ -31,6 +31,8 @@ extern void nf_conntrack_cleanup(struct net *net);
extern int nf_conntrack_proto_init(struct net *net);
extern void nf_conntrack_proto_fini(struct net *net);
+extern void nf_conntrack_cleanup_end(void);
+
extern bool
nf_ct_get_tuple(const struct sk_buff *skb,
unsigned int nhoff,
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 016d95e..e4a0c4f 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1376,11 +1376,12 @@ void nf_conntrack_cleanup(struct net *net)
synchronize_net();
nf_conntrack_proto_fini(net);
nf_conntrack_cleanup_net(net);
+}
- if (net_eq(net, &init_net)) {
- RCU_INIT_POINTER(nf_ct_destroy, NULL);
- nf_conntrack_cleanup_init_net();
- }
+void nf_conntrack_cleanup_end(void)
+{
+ RCU_INIT_POINTER(nf_ct_destroy, NULL);
+ nf_conntrack_cleanup_init_net();
}
void *nf_ct_alloc_hashtable(unsigned int *sizep, int nulls)
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index 363285d..e7185c6 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -575,6 +575,7 @@ static int __init nf_conntrack_standalone_init(void)
static void __exit nf_conntrack_standalone_fini(void)
{
unregister_pernet_subsys(&nf_conntrack_net_ops);
+ nf_conntrack_cleanup_end();
}
module_init(nf_conntrack_standalone_init);
--
1.7.10.4
^ permalink raw reply related
* R: Re: mtu issue with ipsec tunnel and netfilter snat
From: pupilla @ 2013-01-10 16:46 UTC (permalink / raw)
To: jengelh; +Cc: netdev
jengelh@inai.de wrote:
>On Wednesday 2013-01-09 10:01, pupilla@libero.it wrote:
>
>>As you can see there are incoming 1500 bytes packets (these are the
>>decrypted ipsec packets) with DF bit set. These packets are never
>>delivered to the final client 10.81.128.176 (the destination address
>>is 172.16.128.1 which is the ip used for SNATing the original ip
>>10.81.128.176).
>>
>>IMHO this is a mtu issue: 1500 bytes packets cannot be routed inside
>>the ipsec tunnel.
>>
>>But why linux_gw_snat is not sending icmp need to frag packets to
>>10.148.12.23?
>
>Perhaps because ICMP was blocked erroneously?
well, I don't see the icmp packets
because tcpdump 'see' only the incoming
ipsec clear packets. Is there a way to
see the outgoing clear ipsec packets
with tcpdump?
^ permalink raw reply
* Re: [PATCH 0/4] l2tp: fix up network namespace use in l2tp_core
From: Tom Parkin @ 2013-01-10 16:48 UTC (permalink / raw)
To: Tom Parkin; +Cc: netdev
In-Reply-To: <1357756583-22535-1-git-send-email-tparkin@katalix.com>
On 9 January 2013 18:36, Tom Parkin <tparkin@katalix.com> wrote:
> This patchset addresses network namespace issues in l2tp_core as previously
> raised on netdev[1].
Bah, please ignore this changeset for now. I have just discovered a
late change I made to support preemption-enabled kernels breaks the
non-preempt case. I need to reconsider tunnel deletion to properly
address this.
Apologies,
Tom
--
Tom Parkin
www.thesecretdogproject.com
The competent programmer is fully aware of the strictly limited size
of his own skull; therefore he approaches the programming task in full
humility, and among other things he avoids clever tricks like the
plague. /E. W. Dijkstra/
^ permalink raw reply
* [PATCH 1/2] net: ethernet: xilinx: Do not use axienet on PPC
From: Michal Simek @ 2013-01-10 16:58 UTC (permalink / raw)
To: netdev
Axi ethernet can't be used on PPC because it is
little endian IP and PPC is big endian.
This system can't be designed.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
drivers/net/ethernet/xilinx/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/xilinx/Kconfig b/drivers/net/ethernet/xilinx/Kconfig
index 5778a4a..122d60c 100644
--- a/drivers/net/ethernet/xilinx/Kconfig
+++ b/drivers/net/ethernet/xilinx/Kconfig
@@ -27,7 +27,7 @@ config XILINX_EMACLITE
config XILINX_AXI_EMAC
tristate "Xilinx 10/100/1000 AXI Ethernet support"
- depends on (PPC32 || MICROBLAZE)
+ depends on MICROBLAZE
select PHYLIB
---help---
This driver supports the 10/100/1000 Ethernet from Xilinx for the
--
1.7.0.4
^ permalink raw reply related
* [PATCH 2/2] net: ethernet: xilinx: Do not use NO_IRQ in axienet
From: Michal Simek @ 2013-01-10 16:58 UTC (permalink / raw)
To: netdev
In-Reply-To: <1357837123-18996-1-git-send-email-michal.simek@xilinx.com>
This driver is used on Microblaze and will be used
on Arm Zynq.
Microblaze doesn't define NO_IRQ and no IRQ is 0.
Arm still uses NO_IRQ as -1 and there is no option
to connect IRQ to irq 0.
That's why <= 0 is only one option how to find out
undefined IRQ.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index d9f69b8..6f47100 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1590,7 +1590,7 @@ static int axienet_of_probe(struct platform_device *op)
lp->rx_irq = irq_of_parse_and_map(np, 1);
lp->tx_irq = irq_of_parse_and_map(np, 0);
of_node_put(np);
- if ((lp->rx_irq == NO_IRQ) || (lp->tx_irq == NO_IRQ)) {
+ if ((lp->rx_irq <= 0) || (lp->tx_irq <= 0)) {
dev_err(&op->dev, "could not determine irqs\n");
ret = -ENOMEM;
goto err_iounmap_2;
--
1.7.0.4
^ permalink raw reply related
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