Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] per-cgroup tcp buffer limitation
From: Greg Thelen @ 2011-09-09 23:38 UTC (permalink / raw)
  To: Glauber Costa
  Cc: linux-kernel, linux-mm, containers, netdev, xemul,
	David S. Miller, Hiroyouki Kamezawa, Eric W. Biederman,
	Suleiman Souhlal
In-Reply-To: <4E699341.9010606@parallels.com>

On Thu, Sep 8, 2011 at 9:17 PM, Glauber Costa <glommer@parallels.com> wrote:
> On 09/08/2011 06:53 PM, Greg Thelen wrote:
>>
>> If a container must fit within 100M, then a single limit solution
>> would set the limit to 100M and never change it.  In a split limit
>> solution a
>> user daemon (e.g. uswapd) would need to monitor the usage and the amount
>> of
>> active memory vs inactive user memory and unreferenced kernel memory to
>> determine where to apply pressure.
>
> Or it can just define some parameters and let the kernel do the
> rest. Like for instance, a maximum proportion allowed, a maximum
> proportion desired, etc.

Agreed.

>> With some more knobs such a uswapd could
>> attempt to keep ahead of demand.  But eventually direct reclaim would
>> be needed to satisfy rapid growth spikes.  Example: If the 100M container
>> starts with limits of 20M kmem and 80M user memory but later its kernel
>> memory needs grow to 70M.  With separate user and kernel memory
>> limits the kernel memory allocation could fail despite there being
>> reclaimable user pages available.
>
> No no, this is a ratio, not a *limit*. A limit is something you
> should not be allowed to go over. A good limit of kernel memory for
> a 100 Mb container could be something like... 100 mb. But there is
> more to that.
>
> Risking being a bit polemic here, I think that when we do
> containers, we have to view the kernel a little bit like a shared resource
> that is not accounted to anybody. It is easy to account things like tcp
> buffer, but who do you account page tables for shared pages to? Or pinned
> dentries for shared filesystems ?

This is a tough challenge.  If you don't account page tables, then
fork bombs can grab lots of kernel memory.  Containing such pigs is
good.  We are working on some patches that charge page tables to the
memcg they are allocated from.  This charge is checked against the
memcg's limit and thus the page table allocation may fail due if over
limit.

I suggest charging dentries to the cgroup that allocated the dentry.
If containers are used for isolation, then it seems that containers
should typically not share files.  That would break isolation.  Of
course there are shared files (/bin/bash, etc), but at least those are
read-only.

> Being the shared table under everybody, the kernel is more or less
> like buffers inside a physical hdd, or cache lines. You know it is
> there, you know it has a size, but provided you have some sane
> protections, you don't really care - because in most cases you can't
> - who is using it.

I agree that it makes sense to not bother charging fixed sized
resources (struct page, etc.) to containers; they are part of the
platform.  But the set of resources that a process can allocate are
ideally charged to a cgroup to limit container memory usage (i.e.
prevent DoS attacks, isolate performance).

>> The job should have a way to
>> transition to memory limits to 70M+ kernel and 30M- of user.
>
> Yes, and I don't see how what I propose prevents that.

I don't think your current proposal prevents that.  I was thinking
about the future of the proposed kmem cgroup.  I want to make sure the
kernel has a way to apply reclaim pressure bidirectionally between
cgroup kernel memory and cgroup user memory.

>> I suppose a GFP_WAIT slab kernel page allocation could wakeup user space
>> to
>> perform user-assisted direct reclaim.  User space would then lower the
>> user
>> limit thereby causing the kernel to direct reclaim user pages, then
>> the user daemon would raise the kernel limit allowing the slab allocation
>> to
>> succeed.  My hunch is that this would be prone to deadlocks (what prevents
>> uswapd from needing more even more kmem?)  I'll defer to more
>> experienced minds to know if user assisted direct memory reclaim has
>> other pitfalls.  It scares me.
>
> Good that it scares you, it should. OTOH, userspace being
> able to set parameters to it, has nothing scary at all. A daemon in
> userspace can detect that you need more kernel space memory , and
> then - according to a policy you abide to - write to a file allowing
> it more, or maybe not - according to that same policy. It is very
> far away from "userspace driven reclaim".

Agreed.  I have no problem with user space policy being used to
updating kernel control files that alter reclaim.

>> If kmem expands to include reclaimable kernel memory (e.g. dentry) then I
>> presume the kernel would have no way to exchange unused user pages for
>> dentry
>> pages even if the user memory in the container is well below its limit.
>>  This is
>> motivation for the above user assisted direct reclaim.
>
> Dentry is not always reclaimable. If it is pinned, it is non
> reclaimable. Speaking of it, Would you take a look at
> https://lkml.org/lkml/2011/8/14/110 ?
>
> I am targetting dentry as well. But since it is hard to assign a
> dentry to a process all the time, going through a different path. I
> however, haven't entirely given up of doing it cgroups based, so any
> ideas are welcome =)

My hope is that dentry consumption can be effectively limited by
limiting the memory needed to allocate the dentries.  IOW: with a
memcg aware slab allocator which, when possible, charges the calling
process' cgroup.

> Well, I am giving this an extra thought... Having separate knobs
> adds flexibility, but - as usual - also complexity. For the goals I
> have in mind, "kernel memory" would work just as fine.
>
> If you look carefully at the other patches in the series besides
> this one, you'll see that it is just a matter of billing from kernel
> memory instead of tcp-memory, and then all the rest is the same.
>
> Do you think that a single kernel-memory knob would be better for
> your needs? I am willing to give it a try.

Regarding the tcp buffers we're discussing, how does an application
consume lots of buffer memory?  IOW, what would a malicious app do to
cause a grows of tcp buffer usage?  Or is this the kind of resource
that is difficult to directly exploit?

Also, how does the pressure get applied.  I see you have a
per-protocol, per-cgroup pressure pressure setting.  Once set, how
does this pressure cause memory to be freed?  It looks like the
pressure is set when allocating memory and later when packets are
freed the associated memory _might_ be freed if pressure was
previously detected.  Did I get this right?

For me the primary concern is that both user and kernel memory are
eventually charged to the same counter with an associated limit.
Per-container memory pressure is based on this composite limit.  So I
don't have a strong opinion as to how many kernel memory counters
there are so long as they also feed into container memory usage
counter.

>> I agree that kernel memory is somewhat different.  In some (I argue most)
>> situations containers want the ability to exchange job kmem and job umem.
>> Either split or combined accounting protects the system and isolates other
>> containers from kmem allocations of a bad job.  To me it seems natural to
>> indicate that job X gets Y MB of memory.  I have more trouble dividing the
>> Y MB of memory into dedicated slices for different types of memory.
>
> I understand. And I don't think anyone doing containers
> should be mandated to define a division. But a limit...

SGTM, so long as it is an optional kernel memory limit (see below).

>> I think
>> memcg aware slab accounting does a good job of limiting a job's
>> memory allocations.
>> Would such slab accounting meet your needs?
>
> Well, the slab alone, no. There are other objects - like tcp buffers
> - that aren't covered by the slab. Others are usually shared among
> many cgroups, and others don't really belong to anybody in
> particular.igh

Sorry, I am dumb wrt. networking.  I thought that these tcp buffers
were allocated using slab with __alloc_skb().  Are they done in
container process context or arbitrary context?

> How do you think then, about turning this into 2 files inside memcg:
>
>  - kernel_memory_hard_limit.
>  - kernel_memory_soft_limit.
>
> tcp memory would be the one defined in /proc, except if it is
> greater than any of the limits. Instead of testing for memory
> allocation against kmem.tcp_allocated_memory, we'd test it against
> memcg.kmem_pinned_memory.

memcg control files currently include:
  memory.limit_in_bytes
  memory.soft_limit_in_bytes
  memory.usage_in_bytes

If you want a kernel memory limit, then we can introduce two new memcg APIs:
  memory.kernel_limit_in_bytes
  memory.kernel_soft_limit_in_bytes
  memory.kernel_usage_in_bytes

Any memory charged to memory.kernel_limit_in_bytes would also be
charged to memory.limit_in_bytes.  IOW, memory.limit_in_bytes would
include both user pages (as it does today) and some kernel pages.

With these new files there are two cgroup memory limits: total and
kernel.  The total limit is a combination of user and some kinds of
kernel memory.

Kernel page reclaim would occur if memory.kernel_usage_in_bytes
exceeds memory.kernel_limit_in_bytes.  There would be no need to
reclaim user pages in this situation.  If kernel usage exceeds
kernel_soft_limit_in_bytes, then the protocol pressure flags would be
set.

Memcg-wide page reclaim would occur if memory.usage_in_bytes exceeds
memory.limit_in_bytes.  This reclaim would consider either container
kernel memory or user pages associated with the container.

This would be a behavioral memcg change which would need to be
carefully considered.  Today the limit_in_bytes is just a user byte
count that does not include kernel memory.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply

* Re: [PATCH 2/3] netdev/of/phy: Add MDIO bus multiplexer support.
From: Andy Fleming @ 2011-09-09 23:22 UTC (permalink / raw)
  To: David Daney
  Cc: linux-mips, ralf, devicetree-discuss, grant.likely, linux-kernel,
	netdev, David S. Miller
In-Reply-To: <1314820906-14004-3-git-send-email-david.daney@cavium.com>

On Wed, Aug 31, 2011 at 3:01 PM, David Daney <david.daney@cavium.com> wrote:
> This patch adds a somewhat generic framework for MDIO bus
> multiplexers.  It is modeled on the I2C multiplexer.
>
> The multiplexer is needed if there are multiple PHYs with the same
> address connected to the same MDIO bus adepter, or if there is
> insufficient electrical drive capability for all the connected PHY
> devices.
>
> Conceptually it could look something like this:
>
>                   ------------------
>                   | Control Signal |
>                   --------+---------
>                           |
>  ---------------   --------+------
>  | MDIO MASTER |---| Multiplexer |
>  ---------------   --+-------+----
>                     |       |
>                     C       C
>                     h       h
>                     i       i
>                     l       l
>                     d       d
>                     |       |
>     ---------       A       B   ---------
>     |       |       |       |   |       |
>     | PHY@1 +-------+       +---+ PHY@1 |
>     |       |       |       |   |       |
>     ---------       |       |   ---------
>     ---------       |       |   ---------
>     |       |       |       |   |       |
>     | PHY@2 +-------+       +---+ PHY@2 |
>     |       |                   |       |
>     ---------                   ---------
>
> This framework configures the bus topology from device tree data.  The
> mechanics of switching the multiplexer is left to device specific
> drivers.
>
> The follow-on patch contains a multiplexer driven by GPIO lines.


It's amazing how various companies' board designers have come up with
the same brain-dead PHY topologies. We (Freescale) have some similar
code in our tree, but it's not this generically applicable.

>
> Signed-off-by: David Daney <david.daney@cavium.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: "David S. Miller" <davem@davemloft.net>

Looks good to me.

Acked-by: Andy Fleming <afleming@freescale.com>

^ permalink raw reply

* Re: Memory leak in ip_dst_cache
From: Kumar S @ 2011-09-09 22:53 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1315606125.2606.19.camel@edumazet-laptop>

Yes Eric, this command doesn't work on 2.6.24. Which timer are you suspecting?


----- Original Message -----
From: Eric Dumazet <eric.dumazet@gmail.com>
To: Kumar S <ps2kumar@yahoo.com>
Cc: netdev <netdev@vger.kernel.org>
Sent: Friday, September 9, 2011 3:08 PM
Subject: Re: Memory leak in ip_dst_cache

Le vendredi 09 septembre 2011 à 14:58 -0700, Kumar S a écrit :
> I agree, 3 MB is not a concern. But actually this is exceeding, we are
> seeing more than 1200000 objects in a span of 24 hours, which is close
> to 300 MB.

Hmm, please include netdev in your mails.

Then there is something else.

Maybe a timer bug preventing dst_gc_task() to make its duty.

>  Other tasks require somewhere around 200 MB+. When it reaches this
> number, the board reboots. In lab we could simulate this. That is how
> I'm trying to understand the problem. I'm injecting around 10k routes,
> and waiting for them to ageout.
>  

I dont know, you could try disable IP route cache.

On recent (2.6.29+) kernels, thats very easy

# Disable Ip route cache (negative value in rebuild_count)
echo 3000000000 >/proc/sys/net/ipv4/rt_cache_rebuild_count

On an old kernel, you probably need to patch your kernel, or backport
commit 1080d709fb9d8cd43 (net: implement emergency route cache rebulds
when gc_elasticity is exceeded)

^ permalink raw reply

* Re: Backporting the Linux kernel, for good - was: Re: semantic patch inference
From: Luis R. Rodriguez @ 2011-09-09 22:28 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Jesper Andersen, Hauke Mehrtens, linux-wireless, linux-kernel,
	netdev
In-Reply-To: <Pine.LNX.4.64.1109092352230.13368@ask.diku.dk>

On Fri, Sep 9, 2011 at 2:53 PM, Julia Lawall <julia@diku.dk> wrote:
> On Fri, 9 Sep 2011, Luis R. Rodriguez wrote:
>> On Fri, Sep 9, 2011 at 1:48 PM, Julia Lawall <julia@diku.dk> wrote:
>> > I guess that the patch that spdiff will receive will already contain the
>> > appropriate #ifs, so we don't have to be concerned about them.
>>
>> That is correct.
>>
>> > We just add them in as is.
>>
>> I do not follow, add what?
>
> Sorry.  The + code. The #ifdefs ad the compatibility code.  We don't have
> to interpret it, so we don't care whether it is only related to kernel
> version numbers or something more complex.

Ah yes, indeed!

  Luis

^ permalink raw reply

* Re: Memory leak in ip_dst_cache
From: Eric Dumazet @ 2011-09-09 22:08 UTC (permalink / raw)
  To: Kumar S; +Cc: netdev
In-Reply-To: <1315605497.25052.YahooMailNeo@web113916.mail.gq1.yahoo.com>

Le vendredi 09 septembre 2011 à 14:58 -0700, Kumar S a écrit :
> I agree, 3 MB is not a concern. But actually this is exceeding, we are
> seeing more than 1200000 objects in a span of 24 hours, which is close
> to 300 MB.

Hmm, please include netdev in your mails.

Then there is something else.

Maybe a timer bug preventing dst_gc_task() to make its duty.

>  Other tasks require somewhere around 200 MB+. When it reaches this
> number, the board reboots. In lab we could simulate this. That is how
> I'm trying to understand the problem. I'm injecting around 10k routes,
> and waiting for them to ageout.
>  

I dont know, you could try disable IP route cache.

On recent (2.6.29+) kernels, thats very easy

# Disable Ip route cache (negative value in rebuild_count)
echo 3000000000 >/proc/sys/net/ipv4/rt_cache_rebuild_count

On an old kernel, you probably need to patch your kernel, or backport
commit 1080d709fb9d8cd43 (net: implement emergency route cache rebulds
when gc_elasticity is exceeded)

^ permalink raw reply

* Re: Backporting the Linux kernel, for good - was: Re: semantic patch inference
From: Julia Lawall @ 2011-09-09 21:53 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: Jesper Andersen, Hauke Mehrtens, linux-wireless, linux-kernel,
	netdev
In-Reply-To: <CAB=NE6VVaAgyaEX0iTuuj=r1-Vsi5TgAyCj4+3J=QerrJTWs_Q@mail.gmail.com>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1838 bytes --]

On Fri, 9 Sep 2011, Luis R. Rodriguez wrote:

> On Fri, Sep 9, 2011 at 1:48 PM, Julia Lawall <julia@diku.dk> wrote:
> > Thanks for your email.  It made me realize that there was one thing that I
> > didn't understand at all.  If the patches are only intended to apply to
> > linux-next, that makes the problem quite a bit simpler.
> 
> Awesome, and yes the patches/ are only targeted at applying onto
> linux-next.git. When Linus decides to merge and out 3.x-rc1 I simply
> then set $GIT_TREE to $HOME/linux-2.6-allstable/ and run the script to
> suck code from there and apply patches from there.Turns out that
> because the effort was done on linux-next and because linux-next will
> look very much like what Linus ends up merging the patches/ will still
> apply. So what I do then is simply create a branch for that target
> stable kernel and keep refreshing the patches for that stable kernel
> on that branch -- while the master branch keeps chugging along with
> linux-next.
> 
> > I guess that the patch that spdiff will receive will already contain the
> > appropriate #ifs, so we don't have to be concerned about them.
> 
> That is correct.
> 
> > We just add them in as is.
> 
> I do not follow, add what?

Sorry.  The + code. The #ifdefs ad the compatibility code.  We don't have 
to interpret it, so we don't care whether it is only related to kernel 
version numbers or something more complex.

julia

> > There was also the question about one or multiple types of changes.  I
> > think this is not a problem, but Jesper should confirm.  If a patch contains
> > two changes and one can be generalized and the other one cannot for some
> > reason, does spdiff give up on the whole thing, or does it do what it can?
> >
> > Overall, the whole thing seems to be doable :)
> 
> Wow. I'm thrilled, so say the least.
> 
>   Luis
> 

^ permalink raw reply

* Re: Memory leak in ip_dst_cache
From: Eric Dumazet @ 2011-09-09 21:47 UTC (permalink / raw)
  To: Kumar S; +Cc: netdev@vger.kernel.org
In-Reply-To: <1315601283.91185.YahooMailNeo@web113908.mail.gq1.yahoo.com>

Le vendredi 09 septembre 2011 à 13:48 -0700, Kumar S a écrit :
> Hi Eric,
> I did not flush those routes explicitly. They are flushed out
> automatically by calling "rt_run_flush". It inturn calls rt_free(),
> but __cache_free() never gets called after that. Basically, I have 512
> MBytes of memory on board. I want to restrict the cache to around 64k,
> with max_size. But if I do that due to this leak, TCP/IP communication
> stops working when it reaches 64k.
>  

Your dsts use about 3Mb of memory out of 512Mb. Its certainly not the
reason of the failures you have.

Its not a leak.

All "in use" dst are queued in dst_gc, check net/core/dst.c for details.

^ permalink raw reply

* Re: possible routing table cache bug for mtu/advmss attributes in Linux 3.0.4
From: David Madore @ 2011-09-09 21:28 UTC (permalink / raw)
  To: Yan, Zheng ; +Cc: Linux Netdev Mailing-List
In-Reply-To: <CAAM7YA=t=s5EEVFte84oqWaKfqyoNhX_NsE5xWUKJ53u-+Es=g@mail.gmail.com>

On Thu, Sep 08, 2011 at 10:42:20PM +0800, Yan, Zheng  wrote:
> On Fri, Sep 2, 2011 at 5:46 AM, David Madore <david+ml@madore.org> wrote:
> > In a nutshell, the problem is this: my (manually entered) routing
> > tables specify an explicit MTU/MSS for certain IPv6 routes, and after
> > upgrading from 2.6.38.7 to 3.0.4, I found that this MTU/MSS setting is
> > sometimes ignored.  More precisely, it seems that connecting to a host
> > through the route in question causes a routing cache entry to be
> > created, and it is when this cache entry expires that the route
> > forgets the MTU/MSS setting.  (It may also be relevant that in my case
> > the route belongs to a routing table other than "main".)
> 
> Would you please try patch http://marc.info/?l=linux-netdev&m=131529445904858

Indeed, this appears to solve the problem I encountered.  Thanks!

-- 
     David A. Madore
   ( http://www.madore.org/~david/ )

^ permalink raw reply

* Re: Backporting the Linux kernel, for good - was: Re: semantic patch inference
From: Luis R. Rodriguez @ 2011-09-09 21:13 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Jesper Andersen, Hauke Mehrtens, linux-wireless,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <Pine.LNX.4.64.1109092245170.9242-QfmoRoYWmW9knbxzx/v8hQ@public.gmane.org>

On Fri, Sep 9, 2011 at 1:48 PM, Julia Lawall <julia-dAYI7NvHqcQ@public.gmane.org> wrote:
> Thanks for your email.  It made me realize that there was one thing that I
> didn't understand at all.  If the patches are only intended to apply to
> linux-next, that makes the problem quite a bit simpler.

Awesome, and yes the patches/ are only targeted at applying onto
linux-next.git. When Linus decides to merge and out 3.x-rc1 I simply
then set $GIT_TREE to $HOME/linux-2.6-allstable/ and run the script to
suck code from there and apply patches from there.Turns out that
because the effort was done on linux-next and because linux-next will
look very much like what Linus ends up merging the patches/ will still
apply. So what I do then is simply create a branch for that target
stable kernel and keep refreshing the patches for that stable kernel
on that branch -- while the master branch keeps chugging along with
linux-next.

> I guess that the patch that spdiff will receive will already contain the
> appropriate #ifs, so we don't have to be concerned about them.

That is correct.

> We just add them in as is.

I do not follow, add what?

> There was also the question about one or multiple types of changes.  I
> think this is not a problem, but Jesper should confirm.  If a patch contains
> two changes and one can be generalized and the other one cannot for some
> reason, does spdiff give up on the whole thing, or does it do what it can?
>
> Overall, the whole thing seems to be doable :)

Wow. I'm thrilled, so say the least.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation
From: Pavel Emelyanov @ 2011-09-09 20:58 UTC (permalink / raw)
  To: Trond Myklebust
  Cc: Jeff Layton, Stanislav Kinsbursky, linux-nfs@vger.kernel.org,
	neilb@suse.de, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, bfields@fieldses.org,
	davem@davemloft.net
In-Reply-To: <1315596308.17611.46.camel@lade.trondhjem.org>

> Isn't the problem that Stanislav is trying to solve that we need to be
> able to register and unregister RPC services to the correct rpcbind
> server, depending on which net namespace we are in?
> 
> My understanding is that the current code will register everything to
> whatever rpcbind server is running in the init net namespace because
> that's what rpcb_create_local() uses.
> 
> My suggestion is to use a struct pernet_operations to detect when a net
> namespace is being created or destroyed, so that the rpcbind client code
> knows when to create or destroy a connection to the server that is
> running in that namespace.

The problem is that the client has a reference on transport, the transport gets
a socket and the socket holds the net namesace and thus we will not receive the
net->exit event with this approach.

Another option is to break the client's socket reference and kill the rpcb client
on netns stop (i.e. as you propose), but in that case we have another problem - 
how to destroy _other_ clients (not rpc bind ones) when the net namespace stops.

If you prefer either way just let us know, we'll fix the patches.

> Cheers
>   Trond

^ permalink raw reply

* Re: Memory leak in ip_dst_cache
From: Kumar S @ 2011-09-09 20:48 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev@vger.kernel.org
In-Reply-To: <1315596786.2606.3.camel@edumazet-laptop>

Hi Eric,
I did not flush those routes explicitly. They are flushed out automatically by calling "rt_run_flush". It inturn calls rt_free(), but __cache_free() never gets called after that. Basically, I have 512 MBytes of memory on board. I want to restrict the cache to around 64k, with max_size. But if I do that due to this leak, TCP/IP communication stops working when it reaches 64k.
 
Thanks
psk


----- Original Message -----
From: Eric Dumazet <eric.dumazet@gmail.com>
To: Kumar S <ps2kumar@yahoo.com>
Cc: 
Sent: Friday, September 9, 2011 12:33 PM
Subject: Re: Memory leak in ip_dst_cache

Le vendredi 09 septembre 2011 à 11:39 -0700, Kumar S a écrit :
> Hi Eric,
> Thanks for the response. Here is the output of the commands you mentioned.
> 

Hmm, you should have included netdev in this message.

> After injecting around11k routes:
> -sh-2.05b#
> -sh-2.05b# ip route ls cache | wc
>   23256  162772 1166903
> -sh-2.05b#
> 
> After route flush out:
> 
> -sh-2.05b#
> -sh-2.05b# ip route ls cache | wc
>      14      88     710
> -sh-2.05b#
> -sh-2.05b#
> -sh-2.05b#
> -sh-2.05b# cat /proc/slabinfo | grep ip_dst
> ip_dst_cache       11673  11685    256   15    1 : tunables  120   60    0 : slabdata    779    779      0

Thats OK : Each tcp session has a pointer to a dst.

When you "ip route flush cache", busy dst are all invalidated.
"ip route ls cache" doesnt output invalidated dsts.

As soon as tcp session will try to send a packet, it will notice the dst
is obsolete and 'free' it.


> -sh-2.05b#
> -sh-2.05b# grep . /proc/sys/net/ipv4/route/*
> /proc/sys/net/ipv4/route/error_burst:1250
> /proc/sys/net/ipv4/route/error_cost:250
> grep: /proc/sys/net/ipv4/route/flush: Permission denied
> /proc/sys/net/ipv4/route/gc_elasticity:8
> /proc/sys/net/ipv4/route/gc_interval:60
> /proc/sys/net/ipv4/route/gc_min_interval:0
> /proc/sys/net/ipv4/route/gc_min_interval_ms:500
> /proc/sys/net/ipv4/route/gc_thresh:16384
> /proc/sys/net/ipv4/route/gc_timeout:300
> /proc/sys/net/ipv4/route/max_delay:10
> /proc/sys/net/ipv4/route/max_size:2097152
> /proc/sys/net/ipv4/route/min_adv_mss:256
> /proc/sys/net/ipv4/route/min_delay:2
> /proc/sys/net/ipv4/route/min_pmtu:552
> /proc/sys/net/ipv4/route/mtu_expires:600
> /proc/sys/net/ipv4/route/redirect_load:5
> /proc/sys/net/ipv4/route/redirect_number:9
> /proc/sys/net/ipv4/route/redirect_silence:5120
> /proc/sys/net/ipv4/route/secret_interval:600
> -sh-2.05b# rtstat -c10 -i1
> rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|rt_cache|
>  entries|  in_hit|in_slow_|in_slow_|in_no_ro|  in_brd|in_marti|in_marti| out_hit|out_slow|out_slow|gc_total|gc_ignor|gc_goal_|gc_dst_o|in_hlist|out_hlis|
>         |        |     tot|      mc|     ute|        |  an_dst|  an_src|        |    _tot|     _mc|        |      ed|    miss| verflow| _search|t_search|
>    11675|   27700|  101014|       0|       0|     130|       0|       0|   36284|   23436|       0|       0|       0|       0|       0|    9658|   23140|
>    11675|       6|      18|       0|       0|       0|       0|       0|       2|       0|       0|       0|       0|       0|       0|       0|       0|
>    11675|       2|      10|       0|       0|       0|       0|       0|       2|       0|       0|       0|       0|       0|       0|       0|       0|
>    11675|       8|      18|       0|       0|       0|       0|       0|       2|       0|       0|       0|       0|       0|       0|       0|       0|
>    11675|       6|      18|       0|       0|       0|       0|       0|       2|       0|       0|       0|       0|       0|       0|       0|       0|
>    11675|       4|      16|       0|       0|       0|       0|       0|       4|       0|       0|       0|       0|       0|       0|       0|       0|
>    11675|       8|      12|       0|       0|       0|       0|       0|       2|       0|       0|       0|       0|       0|       0|       0|       0|
>    11675|       6|      10|       0|       0|       0|       0|       0|       2|       0|       0|       0|       0|       0|       0|       0|       0|
>    11675|       2|      14|       0|       0|       0|       0|       0|       2|       0|       0|       0|       0|       0|       0|       0|       0|
>    11675|       8|      12|       0|       0|       0|       0|       0|       2|       0|       0|       0|       0|       0|       0|       0|       0|
> -sh-2.05b#
> 
> 

Thats pretty low ip route usage, you dont need to tweak your tunables.

Thanks

^ permalink raw reply

* Re: Draft manpage for recvmmsg
From: Michael Kerrisk @ 2011-09-09 20:52 UTC (permalink / raw)
  To: Andi Kleen
  Cc: linux-man-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
	acme-H+wXaHxf7aLQT0dZR+AlfA, Michael Kerrisk
In-Reply-To: <20101123101551.GA20431-u0/ZJuX+froe6aEkudXLsA@public.gmane.org>

Hi Andi,

On Tue, Nov 23, 2010 at 11:15 AM, Andi Kleen <andi-Vw/NltI1exuRpAAqCnN02g@public.gmane.org> wrote:
>
> Here's a draft manpage for recvmmsg(2), which is one
> of the last undocumented syscalls currently.
> Please review and comment.

Thanks for this page. Can you tell me what license and copyright
should be assigned to it?
See http://man7.org/linux/man-pages/licenses.html

Thanks,

Michael

> -Andi
>
>
> .TH RECVMMSG 2 2010-11-23 "Linux" "Linux Programmer's Manual"
> .SH NAME
> recvmmsg \- receive multiple messages on a socket
> .SH SYNOPSIS
> .BI "#include <sys/socket.h>"
> .br
> .BI "int recvmmsg(int " fd ", struct mmsghdr *" mmsghdr \
> ", unsigned int " vlen ","
> .br
> .BI "             unsigned int " flags ", struct timespec *" timeout ");"
> .SH DESCRIPTION
> The
> .B recvmmsg
> system call receives multiple messages in a socket.
> It acts similar to
> .B recvmsg(2),
> but allows to batch multiple receive operations into a single syscall.
> In addition it support an explicit timeout.
>
> .B fd
> is the file descriptor of the socket to receive data from.
> .B mmsghdr
> is a pointer to an array with length
> .B vlen
> of
> .I mmsghdr
> structures.
> .I struct mmsg
> is defined in
> .I sys/socket.h
> as:
> .in +4n
> .nf
> struct mmsghdr {
>    struct msghdr msg_hdr;  /* Message header */
>    unsigned int  msg_len;  /* Number of received bytes for header */
> };
> .fi
> .in
> .PP
> .B msg_hdr
> is a struct
> .I msghdr
> as described in
> .I recvmsg(2).
> .B msg_len
> is the number of bytes returned for the message in the entry.
> This field has the same value as the return value of a single
> .I recvmsg(2)
> on the header.
>
> .B flags
> contains flags ored together. The flags are the same
> as documented for
> .I recvmsg(2).
> The additional
> .B MSG_WAITFORONE
> turns one
> .I MSG_DONTWAIT
> after the first message has been received.
>
> .B timeout
> points to a
> .I struct timespec
> (see
> .I clock_gettime(2)
> )
> defining a timeout for receiving, or
> .I NULL
> for no timeout. When the timeout expires
> .I recvmmsg
> returns.
> .SH RETURN VALUE
> .I recvmmsg
> returns the number of messages received in
> .I mmsghdr
> or
> -1
> when an error occurs. The
> .I msg_len
> members of
> .I mmsghdr
> are updated for each received message,
> in addition to other fields in the msg_hdr for each message,
> as described in
> .I recvmsg(2).
> .SH SEE ALSO
> .B recvmsg(2),
> .B sendmsg(2),
> .B socket(7),
> .B socket(2),
> .B clock_gettime(2)
> .SH VERSIONS
> The
> .I recvmmsg
> syscall was added with kernel 2.6.32.
> Support in glibc was added with 2.6.12.
> On earlier glibcs the function can be called
> manually using
> .I syscall(2).
>
>
> --
> ak-VuQAYsv1563Yd54FQh9/CA@public.gmane.org -- Speaking for myself only.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-man" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: Backporting the Linux kernel, for good - was: Re: semantic patch inference
From: Julia Lawall @ 2011-09-09 20:48 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: Jesper Andersen, Hauke Mehrtens, linux-wireless,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAB=NE6U0ks9CobukO04=bZpnkm3Yw+Pf7keVPsP-hv1NyJZ_Xw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

Thanks for your email.  It made me realize that there was one thing that I
didn't understand at all.  If the patches are only intended to apply to
linux-next, that makes the problem quite a bit simpler.  I guess that the
patch that spdiff will receive will already contain the appropriate #ifs,
so we don't have to be concerned about them.  We just add them in as is.

There was also the question about one or multiple types of changes.  I
think this is not a problem, but Jesper should confirm.  If a patch contains
two changes and one can be generalized and the other one cannot for some
reason, does spdiff give up on the whole thing, or does it do what it can?

Overall, the whole thing seems to be doable :)

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Backporting the Linux kernel, for good - was: Re: semantic patch inference
From: Luis R. Rodriguez @ 2011-09-09 19:58 UTC (permalink / raw)
  To: Julia Lawall, Jesper Andersen, Hauke Mehrtens
  Cc: linux-wireless, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA

Taking this to the lists for more eyeball review. Some background to
folks who were not at Plumbers or at the 'Backporting the Linux
kernel, for good' BoF:

http://www.linuxplumbersconf.org/2011/ocw/proposals/771
http://www.linuxplumbersconf.org/2011/ocw//system/presentations/771/original/kernel-backport-for-good.odp

In short there are some metrics I have finally evaluated from the
compat work we have. Each backported tarball provides metrics of how
much lines of code we take from upstream, how much work is required
through patches for backport work, and also how many lines of code go
into the generic Linux kernel compatibility module. The talk dissected
these metrics for the first time and provides some conclusions as to
what we can do better, what the estimated cost is of backporting new
subsystems, and some evolutionary practices that we can likely embrace
to make backporting even more automatic which would not only use
spatch but also Jesper's spdiff work. If you are interested in the
metrics stuff please review the slides and if you have questions feel
free to shoot here but below I review with Julia inquiries about
assumptions on how we backport stuff in consideration of spdiff work.

For those curious of spdiff here's a paper on 'Generic Patch
Inference' which explains the logistics behind it:

http://www.diku.dk/~julia/andersen_ase08.pdf

In short, spatch files can be used on target directories to generate
patches. spdiff can read a patch file and generate an spatch file for
you. What this means for the backporting world is if you backport one
evolutionary change in the Linux kernel for one driver you can then
backport the same change for *all* drivers. This is a quantum leap in
terms of effort required to backport.

Anyway, some details on review of assumptions below.

On Fri, Sep 9, 2011 at 6:18 AM, Julia Lawall <julia-dAYI7NvHqcQ@public.gmane.org> wrote:
> Hi,
>
> After some sleep it all seems more clear :)

:)

> First, the choice of patches from linux-next is not our problem.

Exactly! We just

cp -a $COMPAT/ .
cp -a $LINUX_NEXT/subsystem/ .
for i in foo bar
  cp -a drivers/$i .
done

> Second, perhaps we could assume that the patches in compat are always of
> the form
>
> +#ifdef version <= n
> +new code
> +#else
> old code
> +#endif
>
> (and variants - I see that sometimes there is >).  We are going to want to
> treat that as
>
> + new code
> - old code
>
> for the inference process, but I can see that it is very important to have
> the version numbers associated with this information, so that can be
> regenerated in the chosen patch.

Yes, the variance of the #if's expands to other cases as well like:

+#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,39))
        netdev->hw_features = NETIF_F_SG | NETIF_F_HW_VLAN_RX;
+#endif
+#if defined(NETIF_F_HW_VLAN_TX) || (LINUX_VERSION_CODE >=
KERNEL_VERSION(2,6,39))
        netdev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
+#endif

But I'm not even sure why I allowed this to go in, it makes more sense
to remove the flag check on the second if defined. That sort of check
though might be an alternative approach we can check for instead of
kernel versions though, Johannes wanted to see if he can do
backporting this way a while ago but I am not sure up to where he got
up to. Reason for checking for features instead of kernel versions is
some Linux distributions mangle their kernels with features from
future kernels and still keep the same kernel release number. This
makes some kernel backport assumptions incorrect for some of these
distributions and this is why we likely have some of these feature
checks in addition to kernel version checks.

At this point though I feel as if adding a feature label for each
backport work item might be a bit too much work, and sticking to
kernel versions is the way to go. If the work to deal with other
variants is a bit too much then I'd say we can keep those oddball
variants in separate files.

Additionally there is quite a bit of work on the patches/ directory
that can be done to help improve the situation for inspection
purposes. Although each file there does handle specific backport work
items they can be split out even more atomically. Consider the
01-netdev.patch. That has the netdev ops thingy, but also

+#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,6,29))
+       /* can't we remove this? */
+       net->change_mtu = NULL;
+#endif

Note that I am not sure if we can just ignore this, but its just one
example. This random set of bits that do not relate to netdev ops can
be extracted into another file -- unless the work you have may be able
to address multiple work items from one file. If its just targeted for
one work item we can clean this stuff up further to help with that
process.

> Jesper's work takes a patch and the before and after versions of the
> given files in the original code, and finds a semantic patch that takes
> the before code safely to the after code.

:)

> But compat-wireless seems to apply to all versions.

Nope, the patches in the patches/ directory are targeted to apply
cleanly to work in linux-next.git. At times the patches may not apply
cleanly with hunk offsets being off a bit but that is cleaned up
easily through quilt refresh (./scripts/admin-update.sh refresh),
after that the patches will apply cleanly onto linux-next.git.

I should note though that the patches in the patches/ directory
account for backporting effort for *all* supported kernel versions
though. Today my goal is to try at the very least to support down to
the last *supported* 2.6 stable kernel. Work for older kernels is
welcomed though but I think supporting down to the latest stable is a
decent goal.

> So he should interpret the #ifs to figure out what versions a specific change applies to,

Right, to what kernel version, lets call it, a 'backport work item' refers to.

> and then apply the semantic patch inference process to all of those versions?

Yes. But I should note that if the semantic patch inference process
works atomically on only one type of change per reviewed file then
some work is required to split up the patches a bit better. In the
01-netdev.patch we have *most* netdev evolutionary changes in the
kernel that we need to backport, not just one.

> I guess he could take a compat patch and a list of versions, and generate a
> patch with the appropriate
>
> + new code
> - old code
>
> for each of those versions.

Typically the patches address changes for *any* kernel release older
than specific version, splitting the changes for individual kernels is
possible but would be possible but would lead to a bit of redundancy
in work required given that the same change is required for multiple
older supported kernel versions. I will note though that at times the
checks may make some exception to one kernel, but I do not think that
the patches/ directory has any of those, instead I think we have that
dealt with in the compat module -- that is not in patch files.

> Then he could run his tool on those patches and the corresponding versions, and then
> make a semantic patch that includes the appropriate #ifs that could apply to all versions.

Well so in practice the patches/ files address a range of kernels for
which a backport work item is required for can the tool can be made to
infer a set of conditions under which the work item applies to ? In
this case it would be a range of kernel versions.

> If that all seems correct to you, perhaps it is not necessary for you to
> come up from SFO today :)

Ace! I got home at 11pm so it'd be great if I can stay but I do value
this work ****a lot**** so will head out shortly if you think its best
to review in person.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation
From: Trond Myklebust @ 2011-09-09 19:25 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Stanislav Kinsbursky,
	linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Pavel Emelianov, neilb-l3A5Bk7waGM@public.gmane.org,
	netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org,
	davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <20110909150104.5a83c60d-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>

On Fri, 2011-09-09 at 15:01 -0400, Jeff Layton wrote: 
> On Fri, 09 Sep 2011 14:44:34 -0400
> Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > On Fri, 2011-09-09 at 20:41 +0400, Stanislav Kinsbursky wrote: 
> > > 09.09.2011 18:07, Jeff Layton пишет:
> > > > On Fri, 09 Sep 2011 16:08:44 +0400
> > > > Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>  wrote:
> > > >
> > > >> Create rcbind clients or increase rpcbind users counter during RPC service
> > > >> creation and decrease this counter (and possibly destroy those clients) on RPC
> > > >> service destruction.
> > > >>
> > > >> Signed-off-by: Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> > > >>
> > > >> ---
> > > >>   include/linux/sunrpc/clnt.h |    2 ++
> > > >>   net/sunrpc/rpcb_clnt.c      |    2 +-
> > > >>   net/sunrpc/svc.c            |   13 +++++++++++--
> > > >>   3 files changed, 14 insertions(+), 3 deletions(-)
> > > >>
> > > >> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> > > >> index db7bcaf..65a8115 100644
> > > >> --- a/include/linux/sunrpc/clnt.h
> > > >> +++ b/include/linux/sunrpc/clnt.h
> > > >> @@ -135,10 +135,12 @@ void		rpc_shutdown_client(struct rpc_clnt *);
> > > >>   void		rpc_release_client(struct rpc_clnt *);
> > > >>   void		rpc_task_release_client(struct rpc_task *);
> > > >>
> > > >> +int		rpcb_create_local(void);
> > > >>   int		rpcb_register(u32, u32, int, unsigned short);
> > > >>   int		rpcb_v4_register(const u32 program, const u32 version,
> > > >>   				 const struct sockaddr *address,
> > > >>   				 const char *netid);
> > > >> +void		rpcb_put_local(void);
> > > >>   void		rpcb_getport_async(struct rpc_task *);
> > > >>
> > > >>   void		rpc_call_start(struct rpc_task *);
> > > >> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> > > >> index b4cc0f1..437ec60 100644
> > > >> --- a/net/sunrpc/rpcb_clnt.c
> > > >> +++ b/net/sunrpc/rpcb_clnt.c
> > > >> @@ -318,7 +318,7 @@ out:
> > > >>    * Returns zero on success, otherwise a negative errno value
> > > >>    * is returned.
> > > >>    */
> > > >> -static int rpcb_create_local(void)
> > > >> +int rpcb_create_local(void)
> > > >>   {
> > > >>   	static DEFINE_MUTEX(rpcb_create_local_mutex);
> > > >>   	int result = 0;
> > > >> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > > >> index 6a69a11..9095c0e 100644
> > > >> --- a/net/sunrpc/svc.c
> > > >> +++ b/net/sunrpc/svc.c
> > > >> @@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > > >>   	unsigned int xdrsize;
> > > >>   	unsigned int i;
> > > >>
> > > >> -	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> > > >> +	if (rpcb_create_local()<  0)
> > > >>   		return NULL;
> > > >> +
> > > >> +	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> > > >> +		goto out_err;
> > > >>   	serv->sv_name      = prog->pg_name;
> > > >>   	serv->sv_program   = prog;
> > > >>   	serv->sv_nrthreads = 1;
> > > >> @@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > > >>   			GFP_KERNEL);
> > > >>   	if (!serv->sv_pools) {
> > > >>   		kfree(serv);
> > > >> -		return NULL;
> > > >> +		goto out_err;
> > > >>   	}
> > > >>
> > > >>   	for (i = 0; i<  serv->sv_nrpools; i++) {
> > > >> @@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > > >>   	svc_unregister(serv);
> > > >>
> > > >>   	return serv;
> > > >> +
> > > >> +out_err:
> > > >> +	rpcb_put_local();
> > > >> +	return NULL;
> > > >>   }
> > > >>
> > > >>   struct svc_serv *
> > > >> @@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
> > > >>   	svc_unregister(serv);
> > > >>   	kfree(serv->sv_pools);
> > > >>   	kfree(serv);
> > > >> +
> > > >> +	rpcb_put_local();
> > > >>   }
> > > >>   EXPORT_SYMBOL_GPL(svc_destroy);
> > > >>
> > > >>
> > > >
> > > > I don't get it -- what's the advantage of creating rpcbind clients in
> > > > __svc_create vs. the old way of creating them just before we plan to
> > > > use them?
> > > >
> > > 
> > > The main problem here is not in creation, but in destroying those clients.
> > > Now rpcbind clients are created during rpcb_register(). I.e. once per every family, program version and so on.
> > > But can be unregistered for all protocol families by one call. So it's impossible to put reference counting for those clients in the place, where they are created now.
> > 
> > Could we perhaps set up a 'struct pernet_operations' to create a
> > destructor for them?
> > 
> 
> An even easier idea might be to just not take a reference to the
> rpcbind client for svc_programs that have vs_hidden set on every
> version.

Isn't the problem that Stanislav is trying to solve that we need to be
able to register and unregister RPC services to the correct rpcbind
server, depending on which net namespace we are in?

My understanding is that the current code will register everything to
whatever rpcbind server is running in the init net namespace because
that's what rpcb_create_local() uses.

My suggestion is to use a struct pernet_operations to detect when a net
namespace is being created or destroyed, so that the rpcbind client code
knows when to create or destroy a connection to the server that is
running in that namespace.

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: Using gretap to tunnel layer 2 traffic
From: Ben Hutchings @ 2011-09-09 19:19 UTC (permalink / raw)
  To: John H; +Cc: netdev@vger.kernel.org
In-Reply-To: <1315589104.22676.YahooMailNeo@web45104.mail.sp1.yahoo.com>

On Fri, 2011-09-09 at 10:25 -0700, John H wrote:
> I am attempting to tunnel Layer 2 traffic through a gretap device, 
> while encrypting the GRE tunnel with IPsec. My test environment is as follows:
> 
> 10.0.1.1                                          10.0.1.2
> client_a <--> server_left <==> server_right <---> client_b
>                       gretap/IPsec
>                       
> 
> On the servers, I have two VLANs per server, corresponding to the unencrypted
> and encrypted interfaces.  On each server, the unencrypted VLAN is 
> bridged with the gretap device.  All VLANs and physical devices have MTUs of 
> 1500.  The gretap device has a resultant MTU of 1462, thereby causing the 
> bridge device to have an MTU of 1462.
> 
> Traffic for the most part works as it is expected to behave. However, 
> packets are dropped when client_a sends an ICMP packet to client_b which 
> has an MTU less than client_a's device MTU but larger than server_left's 
> MTU. I suspect other protocols would behave similarly (silently dropping
> packets).  I an running "ping -c 1 -s 1450 10.0.1.2" on client_a, this results
> in an ICMP packet being sent with an MTU of 1478.
> 
> An MTU of 1478 is larger than the bridge device's MTU of 1462, causing the 
> packet to be silently discarded per net/bridge/br_forward.c 
> in function br_dev_queue_push_xmit:
>
> int br_dev_queue_push_xmit(struct sk_buff *skb)
> {
>     /* drop mtu oversized packets except gso */
>     if (packet_length(skb) > skb->dev->mtu && !skb_is_gso(skb))
>         kfree_skb(skb);
>     else {
>     ....
>     
> If the gretap device supported GSO, I suspect that this would not be a
> problem. (ethtool -k gretapLeftRight states that GSO/GRO/LRO is not 
> supported)

GRO+GSO may generally be used when forwarding TCP packets.  But aside
from that, none of these have any effect on forwarded packets.

> Function br_dev_queue_push_xmit eventually calls ipgre_tunnel_xmit, if the 
> packet is not dropped.
> 
> I would think that br_dev_queue_push_xmit should call ipgre_tunnel_xmit
> regardless of the device MTU and ipgre_tunnel_xmit would handle packet
> fragmentation internally, but it never has the chance.
> 
> I have tried allowing all packets through br_dev_queue_push_xmit
> and explicitly setting the Don't Frament field to 0 in ipgre_tunnel_xmit,
> but this didn't solve problem.
> 
> Given that this would be tunneling Layer 2 traffic, it wouldn't make sense
> to send an ICMP_FRAG_NEEDED response from the bridge.

Right.

> The real question is, however, why is any client able to send a single ICMP
> packet with size 1478 bytes when one of the hops along the way only 
> supports 1462 bytes per its MTU? Shouldn't this have been negotiated 
> beforehand?

The DHCP and/or route advertisement daemons should tell hosts what the
correct MTU is for the subnet they are on.

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] RDSRDMA: Fix cleanup of rds_iw_mr_pool
From: David Miller @ 2011-09-09 19:05 UTC (permalink / raw)
  To: swise; +Cc: venkat.x.venkatsubra, netdev, rds-devel, jonathan
In-Reply-To: <4E6A62A1.1010702@opengridcomputing.com>

From: Steve Wise <swise@opengridcomputing.com>
Date: Fri, 09 Sep 2011 14:01:53 -0500

> Is this sufficient for you to pull/merge the patch upstream?

It's all queued up in patchwork, so I don't see why not.

^ permalink raw reply

* Re: [PATCH] RDSRDMA: Fix cleanup of rds_iw_mr_pool
From: Steve Wise @ 2011-09-09 19:01 UTC (permalink / raw)
  To: Davem; +Cc: Venkat Venkatsubra, Netdev, Rds-Devel, Jonathan Lallinger
In-Reply-To: <67e08fa0-c2e1-44f8-ab8b-45e4bfdc9264@default>

On 09/09/2011 09:37 AM, Venkat Venkatsubra wrote:
> On 09/08/2011 01:04 PM, Jonathan Lallinger wrote:
>> In the rds_iw_mr_pool struct the free_pinned field keeps track of memory pinned
>> by free MRs. While this field is incremented properly upon allocation, it is never
>> decremented upon unmapping. This would cause the rds_rdma module to crash the
>> kernel upon unloading, by triggering the BUG_ON in the rds_iw_destroy_mr_pool
>> function.
>>
>> This change keeps track of the MRs that become unpinned, so that free_pinned
>> can be decremented appropriately.
>>
>> Signed-off-by: Jonathan Lallinger<jonathan@ogc.us>
>> Signed-off-by: Steve Wise<swise@ogc.us>
> Signed-off-by: Venkat Venkatsubra<venkat.x.venkatsubra@oracle.com>
>
> Venkat
>

Hey Dave,

Is this sufficient for you to pull/merge the patch upstream?  Or should Venkat, the new RDS maintainer, do something 
else?  Like maybe coalesce patches into a git repos and ask you to pull them in?  I'm not sure what process you had with 
the old RDS maintainer.

There's another patch too from Jonathan Lallinger that fixes an RDS crash titled:

[PATCH] RDSRDMA: Fix to PAGE_MASK interpretation

Both have been signed off by Venkat and reviewed by me.


Thanks,

Steve.

^ permalink raw reply

* Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation
From: Jeff Layton @ 2011-09-09 19:01 UTC (permalink / raw)
  To: Trond Myklebust
  Cc: Stanislav Kinsbursky,
	linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Pavel Emelianov, neilb-l3A5Bk7waGM@public.gmane.org,
	netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org,
	davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <1315593874.17611.19.camel-SyLVLa/KEI9HwK5hSS5vWB2eb7JE58TQ@public.gmane.org>

On Fri, 09 Sep 2011 14:44:34 -0400
Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote:

> On Fri, 2011-09-09 at 20:41 +0400, Stanislav Kinsbursky wrote: 
> > 09.09.2011 18:07, Jeff Layton пишет:
> > > On Fri, 09 Sep 2011 16:08:44 +0400
> > > Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>  wrote:
> > >
> > >> Create rcbind clients or increase rpcbind users counter during RPC service
> > >> creation and decrease this counter (and possibly destroy those clients) on RPC
> > >> service destruction.
> > >>
> > >> Signed-off-by: Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> > >>
> > >> ---
> > >>   include/linux/sunrpc/clnt.h |    2 ++
> > >>   net/sunrpc/rpcb_clnt.c      |    2 +-
> > >>   net/sunrpc/svc.c            |   13 +++++++++++--
> > >>   3 files changed, 14 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> > >> index db7bcaf..65a8115 100644
> > >> --- a/include/linux/sunrpc/clnt.h
> > >> +++ b/include/linux/sunrpc/clnt.h
> > >> @@ -135,10 +135,12 @@ void		rpc_shutdown_client(struct rpc_clnt *);
> > >>   void		rpc_release_client(struct rpc_clnt *);
> > >>   void		rpc_task_release_client(struct rpc_task *);
> > >>
> > >> +int		rpcb_create_local(void);
> > >>   int		rpcb_register(u32, u32, int, unsigned short);
> > >>   int		rpcb_v4_register(const u32 program, const u32 version,
> > >>   				 const struct sockaddr *address,
> > >>   				 const char *netid);
> > >> +void		rpcb_put_local(void);
> > >>   void		rpcb_getport_async(struct rpc_task *);
> > >>
> > >>   void		rpc_call_start(struct rpc_task *);
> > >> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> > >> index b4cc0f1..437ec60 100644
> > >> --- a/net/sunrpc/rpcb_clnt.c
> > >> +++ b/net/sunrpc/rpcb_clnt.c
> > >> @@ -318,7 +318,7 @@ out:
> > >>    * Returns zero on success, otherwise a negative errno value
> > >>    * is returned.
> > >>    */
> > >> -static int rpcb_create_local(void)
> > >> +int rpcb_create_local(void)
> > >>   {
> > >>   	static DEFINE_MUTEX(rpcb_create_local_mutex);
> > >>   	int result = 0;
> > >> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > >> index 6a69a11..9095c0e 100644
> > >> --- a/net/sunrpc/svc.c
> > >> +++ b/net/sunrpc/svc.c
> > >> @@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > >>   	unsigned int xdrsize;
> > >>   	unsigned int i;
> > >>
> > >> -	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> > >> +	if (rpcb_create_local()<  0)
> > >>   		return NULL;
> > >> +
> > >> +	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> > >> +		goto out_err;
> > >>   	serv->sv_name      = prog->pg_name;
> > >>   	serv->sv_program   = prog;
> > >>   	serv->sv_nrthreads = 1;
> > >> @@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > >>   			GFP_KERNEL);
> > >>   	if (!serv->sv_pools) {
> > >>   		kfree(serv);
> > >> -		return NULL;
> > >> +		goto out_err;
> > >>   	}
> > >>
> > >>   	for (i = 0; i<  serv->sv_nrpools; i++) {
> > >> @@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> > >>   	svc_unregister(serv);
> > >>
> > >>   	return serv;
> > >> +
> > >> +out_err:
> > >> +	rpcb_put_local();
> > >> +	return NULL;
> > >>   }
> > >>
> > >>   struct svc_serv *
> > >> @@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
> > >>   	svc_unregister(serv);
> > >>   	kfree(serv->sv_pools);
> > >>   	kfree(serv);
> > >> +
> > >> +	rpcb_put_local();
> > >>   }
> > >>   EXPORT_SYMBOL_GPL(svc_destroy);
> > >>
> > >>
> > >
> > > I don't get it -- what's the advantage of creating rpcbind clients in
> > > __svc_create vs. the old way of creating them just before we plan to
> > > use them?
> > >
> > 
> > The main problem here is not in creation, but in destroying those clients.
> > Now rpcbind clients are created during rpcb_register(). I.e. once per every family, program version and so on.
> > But can be unregistered for all protocol families by one call. So it's impossible to put reference counting for those clients in the place, where they are created now.
> 
> Could we perhaps set up a 'struct pernet_operations' to create a
> destructor for them?
> 

An even easier idea might be to just not take a reference to the
rpcbind client for svc_programs that have vs_hidden set on every
version.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH net v2 6/6] bnx2x: Fix ethtool advertisement
From: David Miller @ 2011-09-09 18:44 UTC (permalink / raw)
  To: bhutchings; +Cc: yanivr, eilong, netdev
In-Reply-To: <1315590931.2808.55.camel@bwh-desktop>

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 09 Sep 2011 18:55:31 +0100

> On Wed, 2011-09-07 at 13:48 +0300, Yaniv Rosner wrote:
>> Enable changing advertisement settings via ethtool.
>> 
>> Signed-off-by: Yaniv Rosner <yanivr@broadcom.com>
>> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>
> [...]
> 
> However, I suspect these fixes will not be considered important enough
> for this release cycle, so you may need to be re-spin them for net-next.
> David?

Right, this stuff is not appropriate for the 'net' GIT tree and must
go into 'net-next'.

^ permalink raw reply

* Re: [PATCH v2 3/5] SUNRPC: make RPC service dependable on rpcbind clients creation
From: Trond Myklebust @ 2011-09-09 18:44 UTC (permalink / raw)
  To: Stanislav Kinsbursky
  Cc: Jeff Layton, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Pavel Emelianov, neilb-l3A5Bk7waGM@public.gmane.org,
	netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org,
	davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <4E6A41D4.6090001-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

On Fri, 2011-09-09 at 20:41 +0400, Stanislav Kinsbursky wrote: 
> 09.09.2011 18:07, Jeff Layton пишет:
> > On Fri, 09 Sep 2011 16:08:44 +0400
> > Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>  wrote:
> >
> >> Create rcbind clients or increase rpcbind users counter during RPC service
> >> creation and decrease this counter (and possibly destroy those clients) on RPC
> >> service destruction.
> >>
> >> Signed-off-by: Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> >>
> >> ---
> >>   include/linux/sunrpc/clnt.h |    2 ++
> >>   net/sunrpc/rpcb_clnt.c      |    2 +-
> >>   net/sunrpc/svc.c            |   13 +++++++++++--
> >>   3 files changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> >> index db7bcaf..65a8115 100644
> >> --- a/include/linux/sunrpc/clnt.h
> >> +++ b/include/linux/sunrpc/clnt.h
> >> @@ -135,10 +135,12 @@ void		rpc_shutdown_client(struct rpc_clnt *);
> >>   void		rpc_release_client(struct rpc_clnt *);
> >>   void		rpc_task_release_client(struct rpc_task *);
> >>
> >> +int		rpcb_create_local(void);
> >>   int		rpcb_register(u32, u32, int, unsigned short);
> >>   int		rpcb_v4_register(const u32 program, const u32 version,
> >>   				 const struct sockaddr *address,
> >>   				 const char *netid);
> >> +void		rpcb_put_local(void);
> >>   void		rpcb_getport_async(struct rpc_task *);
> >>
> >>   void		rpc_call_start(struct rpc_task *);
> >> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> >> index b4cc0f1..437ec60 100644
> >> --- a/net/sunrpc/rpcb_clnt.c
> >> +++ b/net/sunrpc/rpcb_clnt.c
> >> @@ -318,7 +318,7 @@ out:
> >>    * Returns zero on success, otherwise a negative errno value
> >>    * is returned.
> >>    */
> >> -static int rpcb_create_local(void)
> >> +int rpcb_create_local(void)
> >>   {
> >>   	static DEFINE_MUTEX(rpcb_create_local_mutex);
> >>   	int result = 0;
> >> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> >> index 6a69a11..9095c0e 100644
> >> --- a/net/sunrpc/svc.c
> >> +++ b/net/sunrpc/svc.c
> >> @@ -367,8 +367,11 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> >>   	unsigned int xdrsize;
> >>   	unsigned int i;
> >>
> >> -	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> >> +	if (rpcb_create_local()<  0)
> >>   		return NULL;
> >> +
> >> +	if (!(serv = kzalloc(sizeof(*serv), GFP_KERNEL)))
> >> +		goto out_err;
> >>   	serv->sv_name      = prog->pg_name;
> >>   	serv->sv_program   = prog;
> >>   	serv->sv_nrthreads = 1;
> >> @@ -403,7 +406,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> >>   			GFP_KERNEL);
> >>   	if (!serv->sv_pools) {
> >>   		kfree(serv);
> >> -		return NULL;
> >> +		goto out_err;
> >>   	}
> >>
> >>   	for (i = 0; i<  serv->sv_nrpools; i++) {
> >> @@ -423,6 +426,10 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> >>   	svc_unregister(serv);
> >>
> >>   	return serv;
> >> +
> >> +out_err:
> >> +	rpcb_put_local();
> >> +	return NULL;
> >>   }
> >>
> >>   struct svc_serv *
> >> @@ -491,6 +498,8 @@ svc_destroy(struct svc_serv *serv)
> >>   	svc_unregister(serv);
> >>   	kfree(serv->sv_pools);
> >>   	kfree(serv);
> >> +
> >> +	rpcb_put_local();
> >>   }
> >>   EXPORT_SYMBOL_GPL(svc_destroy);
> >>
> >>
> >
> > I don't get it -- what's the advantage of creating rpcbind clients in
> > __svc_create vs. the old way of creating them just before we plan to
> > use them?
> >
> 
> The main problem here is not in creation, but in destroying those clients.
> Now rpcbind clients are created during rpcb_register(). I.e. once per every family, program version and so on.
> But can be unregistered for all protocol families by one call. So it's impossible to put reference counting for those clients in the place, where they are created now.

Could we perhaps set up a 'struct pernet_operations' to create a
destructor for them?

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH net v2 6/6] bnx2x: Fix ethtool advertisement
From: Ben Hutchings @ 2011-09-09 17:55 UTC (permalink / raw)
  To: Yaniv Rosner; +Cc: davem, eilong, netdev
In-Reply-To: <1315392491.27750.27.camel@lb-tlvb-dmitry>

On Wed, 2011-09-07 at 13:48 +0300, Yaniv Rosner wrote:
> Enable changing advertisement settings via ethtool.
> 
> Signed-off-by: Yaniv Rosner <yanivr@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>
[...]

However, I suspect these fixes will not be considered important enough
for this release cycle, so you may need to be re-spin them for net-next.
David?

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 net-next v3] af_unix: Fix use-after-free crashes
From: Tim Chen @ 2011-09-09 10:41 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: sedat.dilek, Yan, Zheng, Yan, Zheng, netdev@vger.kernel.org,
	davem@davemloft.net, sfr@canb.auug.org.au, jirislaby@gmail.com,
	Shi, Alex, Valdis Kletnieks
In-Reply-To: <1315551100.5410.30.camel@edumazet-laptop>

On Fri, 2011-09-09 at 08:51 +0200, Eric Dumazet wrote:
> Le jeudi 08 septembre 2011 à 01:37 -0700, Tim Chen a écrit :
> > On Thu, 2011-09-08 at 15:21 +0200, Eric Dumazet wrote:
> > > Le jeudi 08 septembre 2011 à 11:59 +0200, Sedat Dilek a écrit :
> > > 
> > > > I have tested this fixup patch on i386.
> > > > Can we have a separate patch with corrected descriptive text?
> > > > 
> > > > Thanks to all involved people.
> > > 
> > > Here it is :
> > > 
> > > [PATCH net-next v3] af_unix: Fix use-after-free crashes
> > > 
> > > Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
> > > in Unix socket's send and receive path) introduced an use-after-free
> > > bug.
> > > 
> > > We are allowed to steal the references to pid/cred only in the last skb
> > > sent from unix_stream_sendmsg(), because first skbs might be consumed by
> > > the receiver before we finish our sendmsg() call.
> > > 
> > > Remove scm_release() helper, since its cleaner to clear pid/cred fields
> > > in unix_scm_to_skb() when we steal them.
> > > 
> > > Based on prior patches from Yan Zheng and Tim Chen
> > > 
> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > > Reported-by: Jiri Slaby <jirislaby@gmail.com>
> > > Tested-by: Sedat Dilek <sedat.dilek@googlemail.com>
> > > Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
> > > ---
> > 
> > Thanks.
> > 
> > Acked-by: Tim Chen <tim.c.chen@linux.intel.com>
> > 
> 
> Now we have to fix a bug in unix_stream_recvmsg() as well.
> 

Thanks for your careful review to catch the bugs.  Wish I had thought
about the stream msg case more in my original patch.

> consume_skb() call actually releases pid/cred references, and we can use
> them after their eventual freeing.
> 
> Keep also in mind that receiver can provides a too short user buffer,
> and skb can be put back to head of sk_receive_queue
> 
> Tim, your 0856a304091b33 commit introduced a lot of bugs, I was right
> asking a revert.
> 
> If we revert your patch, my litle patch (af_unix: dont send
> SCM_CREDENTIALS by default) is enough to solve performance problems.
> 

But the regression where we do send SCM_CREDENTIALS is not addressed,
right?  I don't mind reverting my patch if there's a better solution.

Tim

^ permalink raw reply

* Re: [PATCH net-next] af_unix: fix use after free in unix_stream_recvmsg()
From: Tim Chen @ 2011-09-09 10:39 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: sedat.dilek, Yan, Zheng, Yan, Zheng, netdev@vger.kernel.org,
	davem@davemloft.net, sfr@canb.auug.org.au, jirislaby@gmail.com,
	Shi, Alex, Valdis Kletnieks
In-Reply-To: <1315555109.2294.9.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>

On Fri, 2011-09-09 at 09:58 +0200, Eric Dumazet wrote:
> Le vendredi 09 septembre 2011 à 08:51 +0200, Eric Dumazet a écrit :
> 
> > Now we have to fix a bug in unix_stream_recvmsg() as well.
> > 
> > consume_skb() call actually releases pid/cred references, and we can use
> > them after their eventual freeing.
> > 
> > Keep also in mind that receiver can provides a too short user buffer,
> > and skb can be put back to head of sk_receive_queue
> > 
> 
> Here is the patch to address this point.
> 
> Apply it after (af_unix: Fix use-after-free crashes)
> 
> I can make a combo patch once everybody agrees.
> 
> [PATCH net-next] af_unix: fix use after free in unix_stream_recvmsg()
> 
> Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
> in Unix socket's send and receive path) introduced an use-after-free
> bug in unix_stream_recvmsg().
> 
> We should call consume_skb(skb) only after our possible use of pid/cred.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/unix/af_unix.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index c8a08ba..1bd4ecf 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1873,6 +1873,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>  	int target;
>  	int err = 0;
>  	long timeo;
> +	struct sk_buff *skb;
>  
>  	err = -EINVAL;
>  	if (sk->sk_state != TCP_ESTABLISHED)
> @@ -1904,7 +1905,6 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>  
>  	do {
>  		int chunk;
> -		struct sk_buff *skb;
>  
>  		unix_state_lock(sk);
>  		skb = skb_dequeue(&sk->sk_receive_queue);
> @@ -1949,6 +1949,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>  			if ((UNIXCB(skb).pid  != siocb->scm->pid) ||
>  			    (UNIXCB(skb).cred != siocb->scm->cred)) {
>  				skb_queue_head(&sk->sk_receive_queue, skb);
> +				skb = NULL;
>  				break;
>  			}
>  		} else {
> @@ -1967,6 +1968,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>  		chunk = min_t(unsigned int, skb->len, size);
>  		if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
>  			skb_queue_head(&sk->sk_receive_queue, skb);
> +			skb = NULL;
>  			if (copied == 0)
>  				copied = -EFAULT;
>  			break;
> @@ -1984,13 +1986,14 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>  			/* put the skb back if we didn't use it up.. */
>  			if (skb->len) {
>  				skb_queue_head(&sk->sk_receive_queue, skb);
> +				skb = NULL;
>  				break;
>  			}
>  
> -			consume_skb(skb);
> -
> -			if (siocb->scm->fp)
> +			if (UNIXCB(skb).pid || siocb->scm->fp)
>  				break;
> +			consume_skb(skb);
> +			skb = NULL;
>  		} else {
>  			/* It is questionable, see note in unix_dgram_recvmsg.
>  			 */
> @@ -1999,12 +2002,14 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>  
>  			/* put message back and return */
>  			skb_queue_head(&sk->sk_receive_queue, skb);
> +			skb = NULL;
>  			break;
>  		}
>  	} while (size);
>  
>  	mutex_unlock(&u->readlock);
>  	scm_recv(sock, msg, siocb->scm, flags);
> +	consume_skb(skb);
>  out:
>  	return copied ? : err;
>  }
> 
> 

Acked-by: Tim Chen <tim.c.chen@linux.intel.com>

^ permalink raw reply

* Using gretap to tunnel layer 2 traffic
From: John H @ 2011-09-09 17:25 UTC (permalink / raw)
  To: netdev@vger.kernel.org

I am attempting to tunnel Layer 2 traffic through a gretap device, 
while encrypting the GRE tunnel with IPsec. My test environment is as follows:

10.0.1.1                                          10.0.1.2
client_a <--> server_left <==> server_right <---> client_b
                      gretap/IPsec
                      

On the servers, I have two VLANs per server, corresponding to the unencrypted
and encrypted interfaces.  On each server, the unencrypted VLAN is 
bridged with the gretap device.  All VLANs and physical devices have MTUs of 
1500.  The gretap device has a resultant MTU of 1462, thereby causing the 
bridge device to have an MTU of 1462.

Traffic for the most part works as it is expected to behave. However, 
packets are dropped when client_a sends an ICMP packet to client_b which 
has an MTU less than client_a's device MTU but larger than server_left's 
MTU. I suspect other protocols would behave similarly (silently dropping
packets).  I an running "ping -c 1 -s 1450 10.0.1.2" on client_a, this results
in an ICMP packet being sent with an MTU of 1478.

An MTU of 1478 is larger than the bridge device's MTU of 1462, causing the 
packet to be silently discarded per net/bridge/br_forward.c 
in function br_dev_queue_push_xmit:

int br_dev_queue_push_xmit(struct sk_buff *skb)
{
    /* drop mtu oversized packets except gso */
    if (packet_length(skb) > skb->dev->mtu && !skb_is_gso(skb))
        kfree_skb(skb);
    else {
    ....
    
If the gretap device supported GSO, I suspect that this would not be a
problem. (ethtool -k gretapLeftRight states that GSO/GRO/LRO is not 
supported)

Function br_dev_queue_push_xmit eventually calls ipgre_tunnel_xmit, if the 
packet is not dropped.

I would think that br_dev_queue_push_xmit should call ipgre_tunnel_xmit
regardless of the device MTU and ipgre_tunnel_xmit would handle packet
fragmentation internally, but it never has the chance.

I have tried allowing all packets through br_dev_queue_push_xmit
and explicitly setting the Don't Frament field to 0 in ipgre_tunnel_xmit,
but this didn't solve problem.

Given that this would be tunneling Layer 2 traffic, it wouldn't make sense
to send an ICMP_FRAG_NEEDED response from the bridge.

The real question is, however, why is any client able to send a single ICMP
packet with size 1478 bytes when one of the hops along the way only 
supports 1462 bytes per its MTU? Shouldn't this have been negotiated 
beforehand?

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox