* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Michael S. Tsirkin @ 2011-05-18 11:17 UTC (permalink / raw)
To: Michał Mirosław
Cc: Shirley Ma, Ben Hutchings, David Miller, Eric Dumazet, Avi Kivity,
Arnd Bergmann, netdev, kvm, linux-kernel
In-Reply-To: <BANLkTimc1RbC3uQsra1HUvS_Trg63iMWGA@mail.gmail.com>
On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> >> > > Hello Michael,
> >> > >
> >> > > Looks like to use a new flag requires more time/work. I am thinking
> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> >> > to
> >> > > avoid the new flag for now since mavctap uses real NICs as lower
> >> > device?
> >> >
> >> > Is there any other restriction besides requiring driver to not recycle
> >> > the skb? Are there any drivers that recycle TX skbs?
> >
> > Not just recycling skbs, keeping reference to any of the pages in the
> > skb. Another requirement is to invoke the callback
> > in a timely fashion. For example virtio-net doesn't limit the time until
> > that happens (skbs are only freed when some other packet is
> > transmitted), so we need to avoid zcopy for such (nested-virt)
> > scenarious, right?
>
> Hmm. But every hardware driver supporting SG will keep reference to
> the pages until the packet is sent (or DMA'd to the device). This can
> take a long time if hardware queue happens to stall for some reason.
That's a fundamental property of zero copy transmit.
You can't let the application/guest reuse the memory until
no one looks at it anymore.
> Is it that you mean keeping a reference after all skbs pointing to the
> pages are released?
No one should reference the pages after the callback is invoked, yes.
> >> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
> >> OK to me from code review.
> > Hmm. pskb_expand_head calls skb_release_data while keeping
> > references to pages. How is that ok? What do I miss?
>
> It's making copy of the skb_shinfo earlier, so the pages refcount
> stays the same.
>
> Best Regards,
> Michał Mirosław
Exactly. But the callback is invoked so the guest thinks it's ok to
change this memory. If it does a corrupted packet will be sent out.
--
MST
^ permalink raw reply
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Michał Mirosław @ 2011-05-18 11:40 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Shirley Ma, Ben Hutchings, David Miller, Eric Dumazet, Avi Kivity,
Arnd Bergmann, netdev, kvm, linux-kernel
In-Reply-To: <20110518111734.GO7589@redhat.com>
W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
<mst@redhat.com> napisał:
> On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
>> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
>> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
>> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
>> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
>> >> > > Hello Michael,
>> >> > >
>> >> > > Looks like to use a new flag requires more time/work. I am thinking
>> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
>> >> > to
>> >> > > avoid the new flag for now since mavctap uses real NICs as lower
>> >> > device?
>> >> >
>> >> > Is there any other restriction besides requiring driver to not recycle
>> >> > the skb? Are there any drivers that recycle TX skbs?
>> >
>> > Not just recycling skbs, keeping reference to any of the pages in the
>> > skb. Another requirement is to invoke the callback
>> > in a timely fashion. For example virtio-net doesn't limit the time until
>> > that happens (skbs are only freed when some other packet is
>> > transmitted), so we need to avoid zcopy for such (nested-virt)
>> > scenarious, right?
>>
>> Hmm. But every hardware driver supporting SG will keep reference to
>> the pages until the packet is sent (or DMA'd to the device). This can
>> take a long time if hardware queue happens to stall for some reason.
>
> That's a fundamental property of zero copy transmit.
> You can't let the application/guest reuse the memory until
> no one looks at it anymore.
>
>> Is it that you mean keeping a reference after all skbs pointing to the
>> pages are released?
> No one should reference the pages after the callback is invoked, yes.
>> >> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
>> >> OK to me from code review.
>> > Hmm. pskb_expand_head calls skb_release_data while keeping
>> > references to pages. How is that ok? What do I miss?
>> It's making copy of the skb_shinfo earlier, so the pages refcount
>> stays the same.
> Exactly. But the callback is invoked so the guest thinks it's ok to
> change this memory. If it does a corrupted packet will be sent out.
Hmm. I tool a quick look at skb_clone(), and it looks like this
sequence will break this scheme:
skb2 = skb_clone(skb...);
kfree_skb(skb) or pskb_expand_head(skb); /* callback called */
[use skb2, pages still referenced]
kfree_skb(skb); /* callback called again */
This sequence is common in bridge, might be in other places.
Maybe this ubuf thing should just track clones? This will make it work
on all devices then.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Eric Dumazet @ 2011-05-18 11:41 UTC (permalink / raw)
To: Jacek Luczak; +Cc: Wei Yongjun, netdev, Vlad Yasevich
In-Reply-To: <BANLkTikVxfxU2uW3AA--q8qt16og=HdDLg@mail.gmail.com>
Le mercredi 18 mai 2011 à 13:01 +0200, Jacek Luczak a écrit :
> @Eric, if you will take a look into the code you might notice
> that there are few places where list operations could be
> optimised and the main question here is do we really care
> to have the data ,,safe'' so that things like that won't popup.
> The good example can be the set of _local_ functions.
> Ahhh... and I'm aware of how tricky can be abuse of RCU.
I took a quick look at existing rcu_read_lock() uses in sctp and did not
find other problems [or optimizations if you prefer this point of view].
Please elaborate.
^ permalink raw reply
* Re: Bug, kernel panic, NULL dereference , cleanup_once / icmp_route_lookup.clone.19.clone / nat , 2.6.39-rc7-git11
From: Eric Dumazet @ 2011-05-18 11:44 UTC (permalink / raw)
To: Denys Fedoryshchenko; +Cc: netdev
In-Reply-To: <1305713111.2991.1.camel@edumazet-laptop>
Le mercredi 18 mai 2011 à 12:05 +0200, Eric Dumazet a écrit :
> Le mercredi 18 mai 2011 à 12:53 +0300, Denys Fedoryshchenko a écrit :
>
> > Yes, i will try. I should enable SLUB debugging only, or also anything
> > else?
> >
> > But possible it will take time to reproduce bug, seems it is occuring
> > rare. With 2.6.39 release i will rollout update to few hundreds PPPoE's,
> > maybe it will increase chances to get information.
> >
>
> I would try both : slub_debug=ZFP slub_nomerge
>
> or maybe only slub_debug=ZFPU
>
> Thanks !
>
Hmm, do you have both ipv6/ipv4 trafic on your machine by any chance ?
^ permalink raw reply
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Michał Mirosław @ 2011-05-18 11:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Shirley Ma, Ben Hutchings, David Miller, Eric Dumazet, Avi Kivity,
Arnd Bergmann, netdev, kvm, linux-kernel
In-Reply-To: <20110518111734.GO7589@redhat.com>
W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
<mst@redhat.com> napisał:
> On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
>> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
>> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
>> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
>> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
>> >> > > Hello Michael,
>> >> > >
>> >> > > Looks like to use a new flag requires more time/work. I am thinking
>> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
>> >> > to
>> >> > > avoid the new flag for now since mavctap uses real NICs as lower
>> >> > device?
>> >> >
>> >> > Is there any other restriction besides requiring driver to not recycle
>> >> > the skb? Are there any drivers that recycle TX skbs?
>> > Not just recycling skbs, keeping reference to any of the pages in the
>> > skb. Another requirement is to invoke the callback
>> > in a timely fashion. For example virtio-net doesn't limit the time until
>> > that happens (skbs are only freed when some other packet is
>> > transmitted), so we need to avoid zcopy for such (nested-virt)
>> > scenarious, right?
>> Hmm. But every hardware driver supporting SG will keep reference to
>> the pages until the packet is sent (or DMA'd to the device). This can
>> take a long time if hardware queue happens to stall for some reason.
> That's a fundamental property of zero copy transmit.
> You can't let the application/guest reuse the memory until
> no one looks at it anymore.
One more question: is userspace (or whatever is sending those packets)
denied from modifying passed pages? I assume it is, but just want to
be sure.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Michael S. Tsirkin @ 2011-05-18 11:47 UTC (permalink / raw)
To: Michał Mirosław
Cc: Shirley Ma, Ben Hutchings, David Miller, Eric Dumazet, Avi Kivity,
Arnd Bergmann, netdev, kvm, linux-kernel
In-Reply-To: <BANLkTi=g_PFfS5653K8ZoQ2Jhp8DhCV1hg@mail.gmail.com>
On Wed, May 18, 2011 at 01:40:29PM +0200, Michał Mirosław wrote:
> W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
> <mst@redhat.com> napisał:
> > On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
> >> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> >> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> >> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> >> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> >> >> > > Hello Michael,
> >> >> > >
> >> >> > > Looks like to use a new flag requires more time/work. I am thinking
> >> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> >> >> > to
> >> >> > > avoid the new flag for now since mavctap uses real NICs as lower
> >> >> > device?
> >> >> >
> >> >> > Is there any other restriction besides requiring driver to not recycle
> >> >> > the skb? Are there any drivers that recycle TX skbs?
> >> >
> >> > Not just recycling skbs, keeping reference to any of the pages in the
> >> > skb. Another requirement is to invoke the callback
> >> > in a timely fashion. For example virtio-net doesn't limit the time until
> >> > that happens (skbs are only freed when some other packet is
> >> > transmitted), so we need to avoid zcopy for such (nested-virt)
> >> > scenarious, right?
> >>
> >> Hmm. But every hardware driver supporting SG will keep reference to
> >> the pages until the packet is sent (or DMA'd to the device). This can
> >> take a long time if hardware queue happens to stall for some reason.
> >
> > That's a fundamental property of zero copy transmit.
> > You can't let the application/guest reuse the memory until
> > no one looks at it anymore.
> >
> >> Is it that you mean keeping a reference after all skbs pointing to the
> >> pages are released?
> > No one should reference the pages after the callback is invoked, yes.
>
> >> >> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
> >> >> OK to me from code review.
> >> > Hmm. pskb_expand_head calls skb_release_data while keeping
> >> > references to pages. How is that ok? What do I miss?
> >> It's making copy of the skb_shinfo earlier, so the pages refcount
> >> stays the same.
> > Exactly. But the callback is invoked so the guest thinks it's ok to
> > change this memory. If it does a corrupted packet will be sent out.
>
> Hmm. I tool a quick look at skb_clone(), and it looks like this
> sequence will break this scheme:
>
> skb2 = skb_clone(skb...);
> kfree_skb(skb) or pskb_expand_head(skb); /* callback called */
> [use skb2, pages still referenced]
> kfree_skb(skb); /* callback called again */
>
> This sequence is common in bridge, might be in other places.
>
> Maybe this ubuf thing should just track clones? This will make it work
> on all devices then.
>
> Best Regards,
> Michał Mirosław
Long term that's a good plan, but it's a lot of work.
pages can also get into weird places like VFS
or devices might hang on to them for a long time.
So I think as a first step, using a flag to white-list
simple devices that don't do any tricks like the
above makes sense.
Just be sure to list all of the restrictions in
the comment where the flag is described.
And hey, we get features extended to 64 bit as a bonus :)
--
MST
^ permalink raw reply
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Michael S. Tsirkin @ 2011-05-18 11:56 UTC (permalink / raw)
To: Michał Mirosław
Cc: Shirley Ma, Ben Hutchings, David Miller, Eric Dumazet, Avi Kivity,
Arnd Bergmann, netdev, kvm, linux-kernel
In-Reply-To: <BANLkTim9RQ94MT4L0QHx-yzZT5nXdbi0aQ@mail.gmail.com>
On Wed, May 18, 2011 at 01:47:33PM +0200, Michał Mirosław wrote:
> W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
> <mst@redhat.com> napisał:
> > On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
> >> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> >> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> >> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> >> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> >> >> > > Hello Michael,
> >> >> > >
> >> >> > > Looks like to use a new flag requires more time/work. I am thinking
> >> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> >> >> > to
> >> >> > > avoid the new flag for now since mavctap uses real NICs as lower
> >> >> > device?
> >> >> >
> >> >> > Is there any other restriction besides requiring driver to not recycle
> >> >> > the skb? Are there any drivers that recycle TX skbs?
> >> > Not just recycling skbs, keeping reference to any of the pages in the
> >> > skb. Another requirement is to invoke the callback
> >> > in a timely fashion. For example virtio-net doesn't limit the time until
> >> > that happens (skbs are only freed when some other packet is
> >> > transmitted), so we need to avoid zcopy for such (nested-virt)
> >> > scenarious, right?
> >> Hmm. But every hardware driver supporting SG will keep reference to
> >> the pages until the packet is sent (or DMA'd to the device). This can
> >> take a long time if hardware queue happens to stall for some reason.
> > That's a fundamental property of zero copy transmit.
> > You can't let the application/guest reuse the memory until
> > no one looks at it anymore.
>
> One more question: is userspace (or whatever is sending those packets)
> denied from modifying passed pages? I assume it is, but just want to
> be sure.
>
> Best Regards,
> Michał Mirosław
Good point.
It's not denied in the sense that it still can modify them if it's
buggy (the pages might not be read-only).
But well-behaved userspace won't modify them until the callback
is invoked.
That would be a problem if the underlying device is
a bridge where we might try to e.g. filter these packets -
data can get modified after the filter. We'd have to copy
whatever the filter accesses and use the copy - it's rarely
the data itself.
That's not normally a problem for macvtap connected to a physical NIC,
as that already bypasses any and all filtering.
But that's another limitation we should note in the comment,
and another reason to limit to specific devices.
--
MST
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Jacek Luczak @ 2011-05-18 11:58 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Wei Yongjun, netdev, Vlad Yasevich
In-Reply-To: <1305718902.2991.3.camel@edumazet-laptop>
2011/5/18 Eric Dumazet <eric.dumazet@gmail.com>:
> Le mercredi 18 mai 2011 à 13:01 +0200, Jacek Luczak a écrit :
>
>> @Eric, if you will take a look into the code you might notice
>> that there are few places where list operations could be
>> optimised and the main question here is do we really care
>> to have the data ,,safe'' so that things like that won't popup.
>> The good example can be the set of _local_ functions.
There should be a vertical space here ...
>> Ahhh... and I'm aware of how tricky can be abuse of RCU.
>
> I took a quick look at existing rcu_read_lock() uses in sctp and did not
> find other problems [or optimizations if you prefer this point of view].
> Please elaborate.
... so that the last line does not have any connection with what I've
wrote above.
I get your point Eric so you don't really have to prove it - if you still need.
-jacek
^ permalink raw reply
* Re: [PATCH 2/2] bna: Add Generic Netlink Interface
From: David Lamparter @ 2011-05-18 12:00 UTC (permalink / raw)
To: Rasesh Mody; +Cc: davem, netdev, adapter_linux_open_src_team, Debashis Dutt
In-Reply-To: <1305694621-28023-3-git-send-email-rmody@brocade.com>
On Tue, May 17, 2011 at 09:57:01PM -0700, Rasesh Mody wrote:
> This patch adds the generic netlink communication interface to BNA driver. The
> in-kernel generic netlink infrastructure can be used to collect hardware
> specific control information and control attributes. The driver makes use of
> the "doit" handler provided by the generic netlink layer to accomplish this.
[...]
> +struct bnad_genl_ioc_info {
> + int status;
> + u16 bnad_num;
> + u16 rsvd;
> + char serialnum[64];
> + char hwpath[BFA_STRING_32];
> + char adapter_hwpath[BFA_STRING_32];
> + char guid[BFA_ADAPTER_SYM_NAME_LEN*2];
> + char name[BFA_ADAPTER_SYM_NAME_LEN];
> + char port_name[BFA_ADAPTER_SYM_NAME_LEN];
> + char eth_name[BFA_ADAPTER_SYM_NAME_LEN];
> + u64 rsvd1[4];
> + mac_t mac;
> + mac_t factory_mac; /* Factory mac address */
> + mac_t current_mac; /* Currently assigned mac address */
> + enum bfa_ioc_type ioc_type;
> + u16 pvid; /* Port vlan id */
> + u16 rsvd2;
> + u32 host;
> + u32 bandwidth; /* For PF support */
> + u32 rsvd3;
> +};
> +
> +struct bnad_genl_ioc_attr {
> + int status;
> + u16 bnad_num;
> + u16 rsvd;
> + struct bfa_ioc_attr ioc_attr;
> +};
These things all look like they're better put into sysfs. Why would you
create a genl protocol just to query some presumably static attributes?
-David
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Jacek Luczak @ 2011-05-18 12:06 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Vlad Yasevich
In-Reply-To: <1305707358.2983.14.camel@edumazet-laptop>
2011/5/18 Eric Dumazet <eric.dumazet@gmail.com>:
> Le mercredi 18 mai 2011 à 10:06 +0200, Jacek Luczak a écrit :
>> 2011/5/18 Eric Dumazet <eric.dumazet@gmail.com>:
>
>> > If you're removing items from this list, you must be a writer here, with
>> > exclusive access. So rcu_read_lock()/rcu_read_unlock() is not necessary.
>>
>> I could agree to some extend ... but strict RCU section IMO is needed here.
>> I can check this if the issue exists.
>>
>
> I can tell you for sure rcu_read_lock() is not needed here. Its only
> showing confusion from code's author.
>
> Please read Documentation/RCU/listRCU.txt for concise explanations,
> line 117.
>
>
>> > Therefore, I guess following code is better :
>> >
>> > list_for_each_entry(addr, &bp->address_list, list) {
>> > list_del_rcu(&addr->list);
>> > call_rcu(&addr->rcu, sctp_local_addr_free);
>> > SCTP_DBG_OBJCNT_DEC(addr);
>> > }
>> >
>> > Then, why dont you fix sctp_bind_addr_clean() instead ?
>> >
>> > if 'struct sctp_sockaddr_entry' is recu protected, then all frees should
>> > be protected as well.
>>
>> The _clean() as claimed by Vlad is called many times from various places
>> in code and this could give a overhead. I guess Vlad would need to comment.
>
> I guess a full review of this code is needed. You'll have to prove
> sctp_bind_addr_clean() is always called after one RCU grace period if
> you want to leave it as is.
Eric this is actually good point which I think did not found at first
glance. As the original
issue occurs between _clean() and _conflict() then if the former is
used here and
there we can hit the grace period not only in that case. By that then
_clean() should
be ,,fixed''. Right?
-Jacek
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Vladislav Yasevich @ 2011-05-18 12:33 UTC (permalink / raw)
To: Wei Yongjun; +Cc: Jacek Luczak, Eric Dumazet, netdev
In-Reply-To: <4DD38B30.9090601@cn.fujitsu.com>
On 05/18/2011 05:02 AM, Wei Yongjun wrote:
> fix the race between sctp_bind_addr_free() and sctp_bind_addr_conflict(), maybe you just
> need to remove the socket from the port hash before empty the bind address list.
> some thing like this, not test.
>
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index c8cc24e..924d846 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -268,12 +268,13 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>
> /* Cleanup. */
> sctp_inq_free(&ep->base.inqueue);
> - sctp_bind_addr_free(&ep->base.bind_addr);
>
> /* Remove and free the port */
> if (sctp_sk(ep->base.sk)->bind_hash)
> sctp_put_port(ep->base.sk);
>
> + sctp_bind_addr_free(&ep->base.bind_addr);
> +
> /* Give up our hold on the sock. */
> if (ep->base.sk)
> sock_put(ep->base.sk);
>
>
I am not sure that this will guarantee avoidance of this crash, even though it is the right
thing to do in general.
We simply make the race condition much smaller and much harder to hit. Potentially, one
cpu may be doing lookup of the socket while the other is doing the destroy. If the lookup cpu
finds the socket just as this code removes the socket from the hash, we still have this potential
race condition.
I agree with Eric, rcu_read_lock() is not strictly necessary, as what we are really after is call_rcu()
based destruction. We need to delay memory destruction for the rcu grace period.
Thinking a little more about how bind_addr_clean() is used, it would probably benefit from getting
converted to call_rcu(). That function is used as local clean-up in case of malloc failure; however,
that doesn't preclude a potential race. The fact that we do not have this race simply points out that
we don't have any kind of parallel lookup that can hit it (and the code confirms it). This doesn't
mean that we wouldn't have it in the future.
-vlad
^ permalink raw reply
* Identifying network namespaces (was: Network namespace manipulation with file descriptors)
From: David Lamparter @ 2011-05-18 12:43 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-arch, linux-kernel, netdev, linux-fsdevel, jamal,
Daniel Lezcano, Linux Containers, Renato Westphal
In-Reply-To: <m1tyd7p7tq.fsf@fess.ebiederm.org>
On Fri, May 06, 2011 at 07:23:29PM -0700, Eric W. Biederman wrote:
> Today there are something things you can use namespaces to implement but
> the userspace code is unnecessarily complex and fragile because of
> limitations of the kernel interfaces.
(I hope I'm not duplicating stuff here, maybe this is already taken care
of...)
There still is the unresolved issue of identifying and/or equality
testing network namespaces. The problems I see are the following:
Problems
========
- there is no simple list of the existing namespaces
most importantly, network namespaces can be abused very sneakily to
provide trojans or other malicious software with a covert
communication channel.
Just create a macvlan device on the local ethernet, shift it into a
new namespace, grab an IP - and the admin of the host will be left
dumbfounded with his plain old netstat or ss as to where those weird
packets are coming from.
(Granted, this needs root privileges and you can still see everything
in /proc/<pid>/net/{tcp,udp,...})
- processes cannot easily be cross referenced with each other
in the case of user space stuff running astray - like management
software crashing, routing daemons screwing up, etc. - it becomes
fairly difficult to shut down a network namespace (or even reaquire
physical devices that have been reassigned)
- namespaces cannot adequately be identified to the user
for debugging, some kernel messages become useless. most prominently,
"unregister_netdevice: waiting for lo to become free. Usage count = 123"
could certainly use some clarification, *which* lo is meant...
So, considering this set of premises (feedback welcome) I looked for
some suitable means of identification. I discarded going for any process
identifiers since Eric's patches allow for network namespaces without
any process holding a reference, using bind mounts instead.
Solution?
=========
I do think I found another identifier suitable for both exporting a full
list and providing "getid" functionality: the "lo" interface index.
I am not fully sure on the rules here, but to my understanding the lo
interface index should be constant for the lifetime of a network
namespace, and furthermore the interface index namespace is global to
the kernel.
This can already be used right now:
- you can identify the init namespace since its lo index is 1
- you can use netlink (or SIOCGIFINDEX, ew.) to get your own ID
- as soon as you have the means to attach to a namespace, you
automatically also have the means to get its ID
What might need to be added:
1) a list of lo indices
2) a way to get a hold of the namespace using its lo index
3) maybe dmesg output on creation/destruction for the abuse case above
4) possibly a file in /proc/<pid>/net that just contains the lo index,
for ease of use
3) and 4) should be trivial; 1) and 2) could be solved by having a
directory in /proc (?) that has all the network namespace "files" with
their lo indices as names.
So, ... Opinions?
-David
^ permalink raw reply
* Re: Bug, kernel panic, NULL dereference , cleanup_once / icmp_route_lookup.clone.19.clone / nat , 2.6.39-rc7-git11
From: Denys Fedoryshchenko @ 2011-05-18 12:46 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1305719047.2991.4.camel@edumazet-laptop>
On Wed, 18 May 2011 13:44:07 +0200, Eric Dumazet wrote:
> Le mercredi 18 mai 2011 à 12:05 +0200, Eric Dumazet a écrit :
>> Le mercredi 18 mai 2011 à 12:53 +0300, Denys Fedoryshchenko a écrit
>> :
>>
>> > Yes, i will try. I should enable SLUB debugging only, or also
>> anything
>> > else?
>> >
>> > But possible it will take time to reproduce bug, seems it is
>> occuring
>> > rare. With 2.6.39 release i will rollout update to few hundreds
>> PPPoE's,
>> > maybe it will increase chances to get information.
>> >
>>
>> I would try both : slub_debug=ZFP slub_nomerge
>>
>> or maybe only slub_debug=ZFPU
>>
>> Thanks !
>>
>
> Hmm, do you have both ipv6/ipv4 trafic on your machine by any chance
> ?
It is NAS, has ipv6 enabled recently (i am preparing for ipv6), but
ipv6 is not routed anywhere, maybe just automatically addresses
appearing on interfaces, including the one looking to customer subnet.
ppp is ipv4 only.
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Jacek Luczak @ 2011-05-18 12:47 UTC (permalink / raw)
To: Vladislav Yasevich; +Cc: Wei Yongjun, Eric Dumazet, netdev
In-Reply-To: <4DD3BC8F.9050802@hp.com>
2011/5/18 Vladislav Yasevich <vladislav.yasevich@hp.com>:
> On 05/18/2011 05:02 AM, Wei Yongjun wrote:
>
>> fix the race between sctp_bind_addr_free() and sctp_bind_addr_conflict(), maybe you just
>> need to remove the socket from the port hash before empty the bind address list.
>> some thing like this, not test.
>>
>> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
>> index c8cc24e..924d846 100644
>> --- a/net/sctp/endpointola.c
>> +++ b/net/sctp/endpointola.c
>> @@ -268,12 +268,13 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>>
>> /* Cleanup. */
>> sctp_inq_free(&ep->base.inqueue);
>> - sctp_bind_addr_free(&ep->base.bind_addr);
>>
>> /* Remove and free the port */
>> if (sctp_sk(ep->base.sk)->bind_hash)
>> sctp_put_port(ep->base.sk);
>>
>> + sctp_bind_addr_free(&ep->base.bind_addr);
>> +
>> /* Give up our hold on the sock. */
>> if (ep->base.sk)
>> sock_put(ep->base.sk);
>>
>>
>
> I am not sure that this will guarantee avoidance of this crash, even though it is the right
> thing to do in general.
>
> We simply make the race condition much smaller and much harder to hit. Potentially, one
> cpu may be doing lookup of the socket while the other is doing the destroy. If the lookup cpu
> finds the socket just as this code removes the socket from the hash, we still have this potential
> race condition.
>
> I agree with Eric, rcu_read_lock() is not strictly necessary, as what we are really after is call_rcu()
> based destruction. We need to delay memory destruction for the rcu grace period.
>
> Thinking a little more about how bind_addr_clean() is used, it would probably benefit from getting
> converted to call_rcu(). That function is used as local clean-up in case of malloc failure; however,
> that doesn't preclude a potential race. The fact that we do not have this race simply points out that
> we don't have any kind of parallel lookup that can hit it (and the code confirms it). This doesn't
> mean that we wouldn't have it in the future.
>
OK then, at the end what Eric suggested is IMO valid:
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index faf71d1..0025d90 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -144,10 +144,9 @@ static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
struct list_head *pos, *temp;
/* Empty the bind address list. */
- list_for_each_safe(pos, temp, &bp->address_list) {
- addr = list_entry(pos, struct sctp_sockaddr_entry, list);
- list_del(pos);
- kfree(addr);
+ list_for_each_entry(pos, &bp->address_list, list) {
+ list_del_rcu(&pos->list);
+ call_rcu(&pos->rcu, sctp_local_addr_free);
SCTP_DBG_OBJCNT_DEC(addr);
}
}
I will test this. Should be safe, avoid race not only in that case and
it consistent.
-Jacek
^ permalink raw reply related
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Michał Mirosław @ 2011-05-18 12:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Shirley Ma, Ben Hutchings, David Miller, Eric Dumazet, Avi Kivity,
Arnd Bergmann, netdev, kvm, linux-kernel
In-Reply-To: <20110518115602.GT7589@redhat.com>
W dniu 18 maja 2011 13:56 użytkownik Michael S. Tsirkin
<mst@redhat.com> napisał:
> On Wed, May 18, 2011 at 01:47:33PM +0200, Michał Mirosław wrote:
>> W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
>> <mst@redhat.com> napisał:
>> > On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
>> >> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
>> >> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
>> >> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
>> >> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
>> >> >> > > Hello Michael,
>> >> >> > >
>> >> >> > > Looks like to use a new flag requires more time/work. I am thinking
>> >> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
>> >> >> > to
>> >> >> > > avoid the new flag for now since mavctap uses real NICs as lower
>> >> >> > device?
>> >> >> >
>> >> >> > Is there any other restriction besides requiring driver to not recycle
>> >> >> > the skb? Are there any drivers that recycle TX skbs?
>> >> > Not just recycling skbs, keeping reference to any of the pages in the
>> >> > skb. Another requirement is to invoke the callback
>> >> > in a timely fashion. For example virtio-net doesn't limit the time until
>> >> > that happens (skbs are only freed when some other packet is
>> >> > transmitted), so we need to avoid zcopy for such (nested-virt)
>> >> > scenarious, right?
>> >> Hmm. But every hardware driver supporting SG will keep reference to
>> >> the pages until the packet is sent (or DMA'd to the device). This can
>> >> take a long time if hardware queue happens to stall for some reason.
>> > That's a fundamental property of zero copy transmit.
>> > You can't let the application/guest reuse the memory until
>> > no one looks at it anymore.
>>
>> One more question: is userspace (or whatever is sending those packets)
>> denied from modifying passed pages? I assume it is, but just want to
>> be sure.
>>
>> Best Regards,
>> Michał Mirosław
>
> Good point.
>
> It's not denied in the sense that it still can modify them if it's
> buggy (the pages might not be read-only).
> But well-behaved userspace won't modify them until the callback
> is invoked.
>
> That would be a problem if the underlying device is
> a bridge where we might try to e.g. filter these packets -
> data can get modified after the filter. We'd have to copy
> whatever the filter accesses and use the copy - it's rarely
> the data itself.
>
> That's not normally a problem for macvtap connected to a physical NIC,
> as that already bypasses any and all filtering.
>
> But that's another limitation we should note in the comment,
> and another reason to limit to specific devices.
It looks like this feature can be used only in very strict circumstances.
What about tcpdump listening on the device or lowerdev? This path
might clone the skb for any device.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Eric Dumazet @ 2011-05-18 12:50 UTC (permalink / raw)
To: Jacek Luczak; +Cc: Vladislav Yasevich, Wei Yongjun, netdev
In-Reply-To: <BANLkTinQAy+jq_VUwky2rxchomuN3avPQg@mail.gmail.com>
Le mercredi 18 mai 2011 à 14:47 +0200, Jacek Luczak a écrit :
> OK then, at the end what Eric suggested is IMO valid:
>
> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
> index faf71d1..0025d90 100644
> --- a/net/sctp/bind_addr.c
> +++ b/net/sctp/bind_addr.c
> @@ -144,10 +144,9 @@ static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
> struct list_head *pos, *temp;
>
> /* Empty the bind address list. */
> - list_for_each_safe(pos, temp, &bp->address_list) {
> - addr = list_entry(pos, struct sctp_sockaddr_entry, list);
> - list_del(pos);
> - kfree(addr);
> + list_for_each_entry(pos, &bp->address_list, list) {
a 'safe' version is needed here, since we remove items in iterator.
> + list_del_rcu(&pos->list);
> + call_rcu(&pos->rcu, sctp_local_addr_free);
> SCTP_DBG_OBJCNT_DEC(addr);
> }
> }
>
>
> I will test this. Should be safe, avoid race not only in that case and
> it consistent.
>
> -Jacek
^ permalink raw reply
* Re: Identifying network namespaces (was: Network namespace manipulation with file descriptors)
From: Alexey Dobriyan @ 2011-05-18 13:03 UTC (permalink / raw)
To: David Lamparter
Cc: Eric W. Biederman, linux-arch, linux-kernel, netdev,
linux-fsdevel, jamal, Daniel Lezcano, Linux Containers,
Renato Westphal
In-Reply-To: <20110518124307.GD3762520@jupiter.n2.diac24.net>
On Wed, May 18, 2011 at 3:43 PM, David Lamparter <equinox@diac24.net> wrote:
> - processes cannot easily be cross referenced with each other
>
> in the case of user space stuff running astray - like management
> software crashing, routing daemons screwing up, etc. - it becomes
> fairly difficult to shut down a network namespace (or even reaquire
> physical devices that have been reassigned)
It shutdowns itself when last process using netns disappeares,
so if you kill your routing daemons you should be fine.
Physical netdevices are moved to init_net.
> - namespaces cannot adequately be identified to the user
>
> for debugging, some kernel messages become useless. most prominently,
> "unregister_netdevice: waiting for lo to become free. Usage count = 123"
> could certainly use some clarification, *which* lo is meant...
There is no "netns %p" or something, because right now the only unique
netns identifier is kernel pointer (which better not be exposed to userspace).
Printing such thing would be quite useless since it's not printed
at netns creation.
> So, considering this set of premises (feedback welcome) I looked for
> some suitable means of identification. I discarded going for any process
> identifiers since Eric's patches allow for network namespaces without
> any process holding a reference, using bind mounts instead.
If anything it should be netns->id, /proc/*/netns outputting id
where id is not derived from kernel pointer.
> Solution?
> =========
What a hack! :-)
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Jacek Luczak @ 2011-05-18 13:11 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Vladislav Yasevich, Wei Yongjun, netdev
In-Reply-To: <1305723046.2991.19.camel@edumazet-laptop>
2011/5/18 Eric Dumazet <eric.dumazet@gmail.com>:
> Le mercredi 18 mai 2011 à 14:47 +0200, Jacek Luczak a écrit :
>
>> OK then, at the end what Eric suggested is IMO valid:
>>
>> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
>> index faf71d1..0025d90 100644
>> --- a/net/sctp/bind_addr.c
>> +++ b/net/sctp/bind_addr.c
>> @@ -144,10 +144,9 @@ static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
>> struct list_head *pos, *temp;
>>
>> /* Empty the bind address list. */
>> - list_for_each_safe(pos, temp, &bp->address_list) {
>> - addr = list_entry(pos, struct sctp_sockaddr_entry, list);
>> - list_del(pos);
>> - kfree(addr);
>> + list_for_each_entry(pos, &bp->address_list, list) {
>
> a 'safe' version is needed here, since we remove items in iterator.
Yep.
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index faf71d1..6150ac5 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -140,14 +140,12 @@ void sctp_bind_addr_init(struct sctp_bind_addr
*bp, __u16 port)
/* Dispose of the address list. */
static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
{
- struct sctp_sockaddr_entry *addr;
- struct list_head *pos, *temp;
+ struct sctp_sockaddr_entry *addr, *temp;
/* Empty the bind address list. */
- list_for_each_safe(pos, temp, &bp->address_list) {
- addr = list_entry(pos, struct sctp_sockaddr_entry, list);
- list_del(pos);
- kfree(addr);
+ list_for_each_entry_safe(addr, temp, &bp->address_list, list) {
+ list_del_rcu(&addr->list);
+ call_rcu(&addr->rcu, sctp_local_addr_free);
SCTP_DBG_OBJCNT_DEC(addr);
}
}
Does it now look good?
-Jacek
^ permalink raw reply related
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Michael S. Tsirkin @ 2011-05-18 13:19 UTC (permalink / raw)
To: Michał Mirosław
Cc: Shirley Ma, Ben Hutchings, David Miller, Eric Dumazet, Avi Kivity,
Arnd Bergmann, netdev, kvm, linux-kernel
In-Reply-To: <BANLkTikOsR9RUJboG1P=oG8ofr5BLbgQ4Q@mail.gmail.com>
On Wed, May 18, 2011 at 02:48:24PM +0200, Michał Mirosław wrote:
> W dniu 18 maja 2011 13:56 użytkownik Michael S. Tsirkin
> <mst@redhat.com> napisał:
> > On Wed, May 18, 2011 at 01:47:33PM +0200, Michał Mirosław wrote:
> >> W dniu 18 maja 2011 13:17 użytkownik Michael S. Tsirkin
> >> <mst@redhat.com> napisał:
> >> > On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
> >> >> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> >> >> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> >> >> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> >> >> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> >> >> >> > > Hello Michael,
> >> >> >> > >
> >> >> >> > > Looks like to use a new flag requires more time/work. I am thinking
> >> >> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> >> >> >> > to
> >> >> >> > > avoid the new flag for now since mavctap uses real NICs as lower
> >> >> >> > device?
> >> >> >> >
> >> >> >> > Is there any other restriction besides requiring driver to not recycle
> >> >> >> > the skb? Are there any drivers that recycle TX skbs?
> >> >> > Not just recycling skbs, keeping reference to any of the pages in the
> >> >> > skb. Another requirement is to invoke the callback
> >> >> > in a timely fashion. For example virtio-net doesn't limit the time until
> >> >> > that happens (skbs are only freed when some other packet is
> >> >> > transmitted), so we need to avoid zcopy for such (nested-virt)
> >> >> > scenarious, right?
> >> >> Hmm. But every hardware driver supporting SG will keep reference to
> >> >> the pages until the packet is sent (or DMA'd to the device). This can
> >> >> take a long time if hardware queue happens to stall for some reason.
> >> > That's a fundamental property of zero copy transmit.
> >> > You can't let the application/guest reuse the memory until
> >> > no one looks at it anymore.
> >>
> >> One more question: is userspace (or whatever is sending those packets)
> >> denied from modifying passed pages? I assume it is, but just want to
> >> be sure.
> >>
> >> Best Regards,
> >> Michał Mirosław
> >
> > Good point.
> >
> > It's not denied in the sense that it still can modify them if it's
> > buggy (the pages might not be read-only).
> > But well-behaved userspace won't modify them until the callback
> > is invoked.
> >
> > That would be a problem if the underlying device is
> > a bridge where we might try to e.g. filter these packets -
> > data can get modified after the filter. We'd have to copy
> > whatever the filter accesses and use the copy - it's rarely
> > the data itself.
> >
> > That's not normally a problem for macvtap connected to a physical NIC,
> > as that already bypasses any and all filtering.
> >
> > But that's another limitation we should note in the comment,
> > and another reason to limit to specific devices.
>
> It looks like this feature can be used only in very strict circumstances.
True. I think it's reasonable to try and start with something
restricted and then add features though - past attempts to solve the problem
generally right away did not bear fruit.
> What about tcpdump listening on the device or lowerdev? This path
> might clone the skb for any device.
>
> Best Regards,
> Michał Mirosław
Thanks for bringing this up: taps do need to be fixed as they can hang
on to a page for unlimited time. Further, as a malicious guest can
change the packet at any time, data that taps get wouldn't be correct.
We can either linearize the problematic skbs or disable
zero copy if there are any taps for the given device.
--
MST
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Eric Dumazet @ 2011-05-18 13:20 UTC (permalink / raw)
To: Jacek Luczak; +Cc: Vladislav Yasevich, Wei Yongjun, netdev
In-Reply-To: <BANLkTi=cDwLFSqaBt5szaFOaWmxYzkya9g@mail.gmail.com>
Le mercredi 18 mai 2011 à 15:11 +0200, Jacek Luczak a écrit :
> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
> index faf71d1..6150ac5 100644
> --- a/net/sctp/bind_addr.c
> +++ b/net/sctp/bind_addr.c
> @@ -140,14 +140,12 @@ void sctp_bind_addr_init(struct sctp_bind_addr
> *bp, __u16 port)
> /* Dispose of the address list. */
> static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
> {
> - struct sctp_sockaddr_entry *addr;
> - struct list_head *pos, *temp;
> + struct sctp_sockaddr_entry *addr, *temp;
>
> /* Empty the bind address list. */
> - list_for_each_safe(pos, temp, &bp->address_list) {
> - addr = list_entry(pos, struct sctp_sockaddr_entry, list);
> - list_del(pos);
> - kfree(addr);
> + list_for_each_entry_safe(addr, temp, &bp->address_list, list) {
> + list_del_rcu(&addr->list);
> + call_rcu(&addr->rcu, sctp_local_addr_free);
> SCTP_DBG_OBJCNT_DEC(addr);
> }
> }
>
> Does it now look good?
>
It seems fine yes !
^ permalink raw reply
* [PATCH net-next-2.6] be2net: Enable SR-IOV for Lancer
From: Padmanabh Ratnakar @ 2011-05-18 13:26 UTC (permalink / raw)
To: davem, netdev; +Cc: Padmanabh Ratnakar, Mammatha Edhala
From: Mammatha Edhala <mammatha.edhala@emulex.com>
Enable SR-IOV for Lancer
Signed-off-by: Mammatha Edhala <mammatha.edhala@emulex.com>
Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@emulex.com>
---
drivers/net/benet/be.h | 5 ++++-
drivers/net/benet/be_main.c | 17 +++++++++--------
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/net/benet/be.h b/drivers/net/benet/be.h
index 0b73dcf..a7db870 100644
--- a/drivers/net/benet/be.h
+++ b/drivers/net/benet/be.h
@@ -49,6 +49,7 @@
#define OC_DEVICE_ID1 0x700 /* Device Id for BE2 cards */
#define OC_DEVICE_ID2 0x710 /* Device Id for BE3 cards */
#define OC_DEVICE_ID3 0xe220 /* Device id for Lancer cards */
+#define OC_DEVICE_ID4 0xe228 /* Device id for VF in Lancer */
static inline char *nic_name(struct pci_dev *pdev)
{
@@ -58,6 +59,7 @@ static inline char *nic_name(struct pci_dev *pdev)
case OC_DEVICE_ID2:
return OC_NAME_BE;
case OC_DEVICE_ID3:
+ case OC_DEVICE_ID4:
return OC_NAME_LANCER;
case BE_DEVICE_ID2:
return BE3_NAME;
@@ -383,7 +385,8 @@ struct be_adapter {
#define BE_GEN2 2
#define BE_GEN3 3
-#define lancer_chip(adapter) (adapter->pdev->device == OC_DEVICE_ID3)
+#define lancer_chip(adapter) ((adapter->pdev->device == OC_DEVICE_ID3) || \
+ (adapter->pdev->device == OC_DEVICE_ID4))
extern const struct ethtool_ops be_ethtool_ops;
diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 7322a51..ce6edac 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -42,6 +42,7 @@ static DEFINE_PCI_DEVICE_TABLE(be_dev_ids) = {
{ PCI_DEVICE(BE_VENDOR_ID, OC_DEVICE_ID1) },
{ PCI_DEVICE(BE_VENDOR_ID, OC_DEVICE_ID2) },
{ PCI_DEVICE(EMULEX_VENDOR_ID, OC_DEVICE_ID3)},
+ { PCI_DEVICE(EMULEX_VENDOR_ID, OC_DEVICE_ID4)},
{ 0 }
};
MODULE_DEVICE_TABLE(pci, be_dev_ids);
@@ -3161,7 +3162,8 @@ static int be_get_config(struct be_adapter *adapter)
memset(mac, 0, ETH_ALEN);
- if (be_physfn(adapter)) {
+ /* A default permanent address is given to each VF for Lancer*/
+ if (be_physfn(adapter) || lancer_chip(adapter)) {
status = be_cmd_mac_addr_query(adapter, mac,
MAC_ADDRESS_TYPE_NETWORK, true /*permanent */, 0);
@@ -3203,6 +3205,7 @@ static int be_dev_family_check(struct be_adapter *adapter)
adapter->generation = BE_GEN3;
break;
case OC_DEVICE_ID3:
+ case OC_DEVICE_ID4:
pci_read_config_dword(pdev, SLI_INTF_REG_OFFSET, &sli_intf);
if_type = (sli_intf & SLI_INTF_IF_TYPE_MASK) >>
SLI_INTF_IF_TYPE_SHIFT;
@@ -3212,10 +3215,6 @@ static int be_dev_family_check(struct be_adapter *adapter)
dev_err(&pdev->dev, "SLI_INTF reg val is not valid\n");
return -EINVAL;
}
- if (num_vfs > 0) {
- dev_err(&pdev->dev, "VFs not supported\n");
- return -EINVAL;
- }
adapter->sli_family = ((sli_intf & SLI_INTF_FAMILY_MASK) >>
SLI_INTF_FAMILY_SHIFT);
adapter->generation = BE_GEN3;
@@ -3381,9 +3380,11 @@ static int __devinit be_probe(struct pci_dev *pdev,
bool link_up;
u16 vf, lnk_speed;
- status = be_vf_eth_addr_config(adapter);
- if (status)
- goto unreg_netdev;
+ if (!lancer_chip(adapter)) {
+ status = be_vf_eth_addr_config(adapter);
+ if (status)
+ goto unreg_netdev;
+ }
for (vf = 0; vf < num_vfs; vf++) {
status = be_cmd_link_status_query(adapter, &link_up,
--
1.6.0.2
^ permalink raw reply related
* [patch 1/1] qeth: use ndo_set_features callback for initial setup and recovery
From: frank.blaschka @ 2011-05-18 13:28 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-s390
In-Reply-To: <20110518132834.728225299@de.ibm.com>
[-- Attachment #1: qeth_fix_hw_feature.patch --]
[-- Type: text/plain, Size: 3128 bytes --]
From: Frank Blaschka <frank.blaschka@de.ibm.com>
This patch uses the ndo_set_features callback during normal device
startup or recovery to turn on hardware RX checksum. Patch was done
with much help from Michal Miroslaw, thx!!!
Signed-off-by: Frank Blaschka <frank.blaschka@de.ibm.com>
---
drivers/s390/net/qeth_l3_main.c | 77 ++++++++++++----------------------------
1 file changed, 25 insertions(+), 52 deletions(-)
--- a/drivers/s390/net/qeth_l3_main.c
+++ b/drivers/s390/net/qeth_l3_main.c
@@ -1417,63 +1417,33 @@ int qeth_l3_set_rx_csum(struct qeth_card
int rc = 0;
if (on) {
- if (card->state != CARD_STATE_DOWN) {
- if (!qeth_is_supported(card,
- IPA_INBOUND_CHECKSUM))
- return -EPERM;
- rc = qeth_l3_send_checksum_command(card);
- if (rc)
- return -EIO;
- }
- card->dev->features |= NETIF_F_RXCSUM;
+ rc = qeth_l3_send_checksum_command(card);
+ if (rc)
+ return -EIO;
+ dev_info(&card->gdev->dev,
+ "HW Checksumming (inbound) enabled\n");
} else {
- if (card->state != CARD_STATE_DOWN) {
- rc = qeth_l3_send_simple_setassparms(card,
- IPA_INBOUND_CHECKSUM, IPA_CMD_ASS_STOP, 0);
- if (rc)
- return -EIO;
- }
- card->dev->features &= ~NETIF_F_RXCSUM;
+ rc = qeth_l3_send_simple_setassparms(card,
+ IPA_INBOUND_CHECKSUM, IPA_CMD_ASS_STOP, 0);
+ if (rc)
+ return -EIO;
}
- return rc;
+ return 0;
}
static int qeth_l3_start_ipa_checksum(struct qeth_card *card)
{
- int rc = 0;
-
QETH_CARD_TEXT(card, 3, "strtcsum");
if (card->dev->features & NETIF_F_RXCSUM) {
- /* hw may have changed during offline or recovery */
- if (!qeth_is_supported(card, IPA_INBOUND_CHECKSUM)) {
- dev_info(&card->gdev->dev,
- "Inbound HW Checksumming not "
- "supported on %s,\ncontinuing "
- "using Inbound SW Checksumming\n",
- QETH_CARD_IFNAME(card));
- goto update_feature;
- }
-
- rc = qeth_l3_send_checksum_command(card);
- if (!rc)
- dev_info(&card->gdev->dev,
- "HW Checksumming (inbound) enabled\n");
- else
- goto update_feature;
- } else
- dev_info(&card->gdev->dev,
- "Using SW checksumming on %s.\n",
- QETH_CARD_IFNAME(card));
+ rtnl_lock();
+ /* force set_features call */
+ card->dev->features &= ~NETIF_F_RXCSUM;
+ netdev_update_features(card->dev);
+ rtnl_unlock();
+ }
return 0;
-
-update_feature:
- rtnl_lock();
- card->dev->features &= ~NETIF_F_RXCSUM;
- netdev_update_features(card->dev);
- rtnl_unlock();
- return rc;
}
static int qeth_l3_start_ipa_tx_checksum(struct qeth_card *card)
@@ -3196,17 +3166,20 @@ static int qeth_l3_set_features(struct n
{
struct qeth_card *card = dev->ml_priv;
u32 changed = dev->features ^ features;
- int on;
+ int err;
if (!(changed & NETIF_F_RXCSUM))
return 0;
- if (features & NETIF_F_RXCSUM)
- on = 1;
- else
- on = 0;
+ if (card->state == CARD_STATE_DOWN ||
+ card->state == CARD_STATE_RECOVER)
+ return 0;
+
+ err = qeth_l3_set_rx_csum(card, features & NETIF_F_RXCSUM);
+ if (err)
+ dev->features = features ^ NETIF_F_RXCSUM;
- return qeth_l3_set_rx_csum(card, on);
+ return err;
}
static const struct ethtool_ops qeth_l3_ethtool_ops = {
^ permalink raw reply
* [patch 0/1] s390: one more qeth patch for net-next
From: frank.blaschka @ 2011-05-18 13:28 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-s390
Hi Dave,
thanks to the help and patience of Michal Miroslaw here is a
patch to finalize the qeth hw_features rework.
shortlog:
Frank Blaschka (1)
qeth: use ndo_set_features callback for initial setup and recovery
Thanks,
Frank
^ permalink raw reply
* Re: Identifying network namespaces (was: Network namespace manipulation with file descriptors)
From: David Lamparter @ 2011-05-18 13:33 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: linux-arch-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Linux Containers,
Eric W. Biederman, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
David Lamparter
In-Reply-To: <BANLkTikmrC86hk=W84UBwhJLe_uGAN4w9w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, May 18, 2011 at 04:03:03PM +0300, Alexey Dobriyan wrote:
> On Wed, May 18, 2011 at 3:43 PM, David Lamparter <equinox@diac24.net> wrote:
> > - processes cannot easily be cross referenced with each other
> >
> > in the case of user space stuff running astray - like management
> > software crashing, routing daemons screwing up, etc. - it becomes
> > fairly difficult to shut down a network namespace (or even reaquire
> > physical devices that have been reassigned)
>
> It shutdowns itself when last process using netns disappeares,
> so if you kill your routing daemons you should be fine.
> Physical netdevices are moved to init_net.
Now assume I'm running pptpd, which forks a new pppd for each
connection. Even if I kill pptpd, the pppd keeps running... now how do I
find the pppds that belong to that one namespace that I'm trying to
get rid of?
> > - namespaces cannot adequately be identified to the user
> >
> > for debugging, some kernel messages become useless. most prominently,
> > "unregister_netdevice: waiting for lo to become free. Usage count = 123"
> > could certainly use some clarification, *which* lo is meant...
>
> There is no "netns %p" or something, because right now the only unique
> netns identifier is kernel pointer (which better not be exposed to userspace).
> Printing such thing would be quite useless since it's not printed
> at netns creation.
I agree printing the kernel pointer is point-less.
> > So, considering this set of premises (feedback welcome) I looked for
> > some suitable means of identification. I discarded going for any process
> > identifiers since Eric's patches allow for network namespaces without
> > any process holding a reference, using bind mounts instead.
>
> If anything it should be netns->id, /proc/*/netns outputting id
> where id is not derived from kernel pointer.
>
> > Solution?
> > [ using lo interface index ]
>
> What a hack! :-)
Well, you could create another counter and count it up on namespace
creation. But the interface index is readily available to userspace as
is, and it uniquely identifies the network namespace.
(Stupidest thing you can do to break this is renaming the loopback
device; but even if you do that userspace can still look at the LOOPBACK
flag.)
-David
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Jacek Luczak @ 2011-05-18 13:39 UTC (permalink / raw)
To: Vladislav Yasevich; +Cc: Eric Dumazet, Wei Yongjun, netdev
In-Reply-To: <4DD3CA57.5060709@hp.com>
2011/5/18 Vladislav Yasevich <vladislav.yasevich@hp.com>:
> On 05/18/2011 09:11 AM, Jacek Luczak wrote:
>> 2011/5/18 Eric Dumazet <eric.dumazet@gmail.com>:
>>> Le mercredi 18 mai 2011 à 14:47 +0200, Jacek Luczak a écrit :
>>>
>>>> OK then, at the end what Eric suggested is IMO valid:
>>>>
>>>> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
>>>> index faf71d1..0025d90 100644
>>>> --- a/net/sctp/bind_addr.c
>>>> +++ b/net/sctp/bind_addr.c
>>>> @@ -144,10 +144,9 @@ static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
>>>> struct list_head *pos, *temp;
>>>>
>>>> /* Empty the bind address list. */
>>>> - list_for_each_safe(pos, temp, &bp->address_list) {
>>>> - addr = list_entry(pos, struct sctp_sockaddr_entry, list);
>>>> - list_del(pos);
>>>> - kfree(addr);
>>>> + list_for_each_entry(pos, &bp->address_list, list) {
>>>
>>> a 'safe' version is needed here, since we remove items in iterator.
>>
>> Yep.
>>
>> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
>> index faf71d1..6150ac5 100644
>> --- a/net/sctp/bind_addr.c
>> +++ b/net/sctp/bind_addr.c
>> @@ -140,14 +140,12 @@ void sctp_bind_addr_init(struct sctp_bind_addr
>> *bp, __u16 port)
>> /* Dispose of the address list. */
>> static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
>> {
>> - struct sctp_sockaddr_entry *addr;
>> - struct list_head *pos, *temp;
>> + struct sctp_sockaddr_entry *addr, *temp;
>>
>> /* Empty the bind address list. */
>> - list_for_each_safe(pos, temp, &bp->address_list) {
>> - addr = list_entry(pos, struct sctp_sockaddr_entry, list);
>> - list_del(pos);
>> - kfree(addr);
>> + list_for_each_entry_safe(addr, temp, &bp->address_list, list) {
>> + list_del_rcu(&addr->list);
>> + call_rcu(&addr->rcu, sctp_local_addr_free);
>> SCTP_DBG_OBJCNT_DEC(addr);
>> }
>> }
>>
>> Does it now look good?
>
> Yes. It should the fix the race.
>
Thanks guys then for your guidance. I will repost final patch.
-Jacek
^ 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