Change in alloc_skb() behavior in 3.2+ kernels?

Change in alloc_skb() behavior in 3.2+ kernels?

[Grant Edwards]

I'm tracking down a problem that appears to be caused by a change in
the behavior of alloc_skb() introduced in kernel version 3.2.  In
kernel versions prior to 3.2, calling alloc_skb(1350), returned an
sk_buff with a tailroom of around 1400 bytes (safely below the default
Ethernet frame size limit of 1500).

In 3.2 and later, calling alloc_skb(1350) returns an sk_buff with a
tailroom of about 1850.

Why has the "extra" space increased from 60 bytes to 500 bytes?

[It's always possible that I've unintentionally changed something in
the kernel configs that causes this, but I've tried to build the
kernels as identically as possible.]

The kernel module that's started failing fills the allocated sk_buff
until tailroom() indicates it is full and then sends it.  The problem
is that sending a packet with a length of 1850 won't work (it's a
MAC-layer Ethernet packet).

I've found man pages for alloc_skb() from a few years ago that state
explicitly that alloc_skb(_size_) will allocate a new sk_buff with no
headroom and a tail room of _size_ bytes.  This doesn't seem to be the
case for recent kernels.  Is there any documentation stating what the
current behavior is supposed to be?

Are callers to alloc_skb() supposed to check the tailroom and
reserve() an appropriate number of bytes such that the tailroom is
correct?

Is the tailroom of the allocated sk_buff guaranteed to be at least as
large as the requested size, or does application code also have to
check for tailroom less than the requested size?

The ultimate question I'm trying to answer is what is the "right" way
to allocate an sk_buff that has a size appropriate for an Ethernet
frame assuming an MTU of 1500?

-- 
Grant Edwards               grant.b.edwards        Yow! Let's send the
                                  at               Russians defective
                              gmail.com            lifestyle accessories!

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Eric Dumazet]

On Wed, 2012-06-06 at 18:32 +0000, Grant Edwards wrote:
> I'm tracking down a problem that appears to be caused by a change in
> the behavior of alloc_skb() introduced in kernel version 3.2.  In
> kernel versions prior to 3.2, calling alloc_skb(1350), returned an
> sk_buff with a tailroom of around 1400 bytes (safely below the default
> Ethernet frame size limit of 1500).
> 
> In 3.2 and later, calling alloc_skb(1350) returns an sk_buff with a
> tailroom of about 1850.
> 
> Why has the "extra" space increased from 60 bytes to 500 bytes?
> 

Because of kmalloc-2048 being used. Previous kernels were losing this
space. We are now able to expand some packets without extra
re-allocation/copy.

> [It's always possible that I've unintentionally changed something in
> the kernel configs that causes this, but I've tried to build the
> kernels as identically as possible.]
> 
> The kernel module that's started failing fills the allocated sk_buff
> until tailroom() indicates it is full and then sends it.  The problem
> is that sending a packet with a length of 1850 won't work (it's a
> MAC-layer Ethernet packet).

This code seems buggy.

> 
> I've found man pages for alloc_skb() from a few years ago that state
> explicitly that alloc_skb(_size_) will allocate a new sk_buff with no
> headroom and a tail room of _size_ bytes.  This doesn't seem to be the
> case for recent kernels.  Is there any documentation stating what the
> current behavior is supposed to be?
> 
> Are callers to alloc_skb() supposed to check the tailroom and
> reserve() an appropriate number of bytes such that the tailroom is
> correct?
> 

If you allocate skbs with 1500 bytes, you probably should check skb->len
more than tailroom...

> Is the tailroom of the allocated sk_buff guaranteed to be at least as
> large as the requested size, or does application code also have to
> check for tailroom less than the requested size?
> 
> The ultimate question I'm trying to answer is what is the "right" way
> to allocate an sk_buff that has a size appropriate for an Ethernet
> frame assuming an MTU of 1500?
> 

I dont know what to answer. Could you point the code in question ?

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[David Miller]

From: Grant Edwards <grant.b.edwards@gmail.com>
Date: Wed, 6 Jun 2012 18:32:57 +0000 (UTC)

> The kernel module that's started failing fills the allocated sk_buff
> until tailroom() indicates it is full and then sends it.  The problem
> is that sending a packet with a length of 1850 won't work (it's a
> MAC-layer Ethernet packet).

The amount of tailroom an SKB has is implementation dependent.

It's incredibly poor form to rely upon it to determine whether a fully
sized frame has been constructed or not.

Please fix the code that does this.

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Grant Edwards]

On 2012-06-06, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2012-06-06 at 18:32 +0000, Grant Edwards wrote:
>
>> I'm tracking down a problem that appears to be caused by a change in
>> the behavior of alloc_skb() introduced in kernel version 3.2.  In
>> kernel versions prior to 3.2, calling alloc_skb(1350), returned an
>> sk_buff with a tailroom of around 1400 bytes (safely below the
>> default Ethernet frame size limit of 1500).
>> 
>> In 3.2 and later, calling alloc_skb(1350) returns an sk_buff with a
>> tailroom of about 1850.
>> 
>> Why has the "extra" space increased from 60 bytes to 500 bytes?
>
> Because of kmalloc-2048 being used. Previous kernels were losing this
> space. We are now able to expand some packets without extra
> re-allocation/copy.
>
>> [It's always possible that I've unintentionally changed something in
>> the kernel configs that causes this, but I've tried to build the
>> kernels as identically as possible.]
>> 
>> The kernel module that's started failing fills the allocated sk_buff
>> until tailroom() indicates it is full and then sends it.  The problem
>> is that sending a packet with a length of 1850 won't work (it's a
>> MAC-layer Ethernet packet).
>
> This code seems buggy.

It is with today's alloc_skb().

At the time it was written (probably 10+ years ago) it was relying on
the documented API for alloc_skb() that stated alloc_skb() either
returned an sk_buff of the requested size or it failed.

>> I've found man pages for alloc_skb() from a few years ago that state
>> explicitly that alloc_skb(_size_) will allocate a new sk_buff with no
>> headroom and a tail room of _size_ bytes.  This doesn't seem to be the
>> case for recent kernels.  Is there any documentation stating what the
>> current behavior is supposed to be?
>> 
>> Are callers to alloc_skb() supposed to check the tailroom and
>> reserve() an appropriate number of bytes such that the tailroom is
>> correct?
>
> If you allocate skbs with 1500 bytes, you probably should check skb->len
> more than tailroom...

I can do that -- assuming it's backwards compatible with older kernels
as well (let's say as far back as 2.6 -- we stopped trying to support
2.4 kernels last year).

>> Is the tailroom of the allocated sk_buff guaranteed to be at least as
>> large as the requested size, or does application code also have to
>> check for tailroom less than the requested size?
>> 
>> The ultimate question I'm trying to answer is what is the "right" way
>> to allocate an sk_buff that has a size appropriate for an Ethernet
>> frame assuming an MTU of 1500?
>
> I dont know what to answer. Could you point the code in question?

Sure:

   alloc_skb(dev->hard_header_len + 1340)

The intent was to allocate a frame that we can guarantee we can send
out via the Ethernet device 'dev'.  Sometime 12-14 years ago, somebody
pulled the number "1340" out of the air under the assumption that the
resulting sk_buff would always be safely under the Ethernet limit of
1500 bytes.

That worked until kernel 3.2 came out, at which time we started ending
up with 1800 byte frames.

-- 
Grant Edwards               grant.b.edwards        Yow! I've read SEVEN
                                  at               MILLION books!!
                              gmail.com            

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Grant Edwards]

On 2012-06-06, David Miller <davem@davemloft.net> wrote:
> From: Grant Edwards <grant.b.edwards@gmail.com>
> Date: Wed, 6 Jun 2012 18:32:57 +0000 (UTC)
>
>> The kernel module that's started failing fills the allocated sk_buff
>> until tailroom() indicates it is full and then sends it.  The problem
>> is that sending a packet with a length of 1850 won't work (it's a
>> MAC-layer Ethernet packet).
>
> The amount of tailroom an SKB has is implementation dependent.

Today, that's apparently the case, but that's not what the man page
for alloc_skb said when the code was written.

> It's incredibly poor form to rely upon it to determine whether a fully
> sized frame has been constructed or not.
>
> Please fix the code that does this.

That's what I'll do as soon as I can find a definition of what the API
for alloc_skb() actually _is_.  It has clearly changed in the past few
years.

-- 
Grant Edwards               grant.b.edwards        Yow! I'm also against
                                  at               BODY-SURFING!!
                              gmail.com            

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[David Miller]

From: Grant Edwards <grant.b.edwards@gmail.com>
Date: Wed, 6 Jun 2012 18:59:19 +0000 (UTC)

> At the time it was written (probably 10+ years ago) it was relying on
> the documented API for alloc_skb() that stated alloc_skb() either
> returned an sk_buff of the requested size or it failed.

It was never a formal API that we would only allocate 'size'
amount of tailroom.

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Eric Dumazet]

On Wed, 2012-06-06 at 11:51 -0700, David Miller wrote:
> From: Grant Edwards <grant.b.edwards@gmail.com>
> Date: Wed, 6 Jun 2012 18:32:57 +0000 (UTC)
> 
> > The kernel module that's started failing fills the allocated sk_buff
> > until tailroom() indicates it is full and then sends it.  The problem
> > is that sending a packet with a length of 1850 won't work (it's a
> > MAC-layer Ethernet packet).
> 
> The amount of tailroom an SKB has is implementation dependent.
> 
> It's incredibly poor form to rely upon it to determine whether a fully
> sized frame has been constructed or not.
> 
> Please fix the code that does this.


By the way, we had a similar problem, and the fix was :

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=a21d45726acacc963d8baddf74607d9b74e2b723

Grant, depending on the context, you might use skb->avail_size and
skb_availroom() as well.

Beware skb->avail_size is unioned with skb->{mark|dropcount}

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[David Miller]

From: Grant Edwards <grant.b.edwards@gmail.com>
Date: Wed, 6 Jun 2012 19:01:40 +0000 (UTC)

> That's what I'll do as soon as I can find a definition of what the API
> for alloc_skb() actually _is_.  It has clearly changed in the past few
> years.

And it will continue to change.  There are no stable APIs inside of
the kernel, none.

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Grant Edwards]

On 2012-06-06, David Miller <davem@davemloft.net> wrote:
> From: Grant Edwards <grant.b.edwards@gmail.com>
> Date: Wed, 6 Jun 2012 18:59:19 +0000 (UTC)
>
>> At the time it was written (probably 10+ years ago) it was relying on
>> the documented API for alloc_skb() that stated alloc_skb() either
>> returned an sk_buff of the requested size or it failed.
>
> It was never a formal API that we would only allocate 'size'
> amount of tailroom.

Well, somebody forgot to tell whoever wrote the man page for
alloc_skb().  :)

-- 
Grant Edwards               grant.b.edwards        Yow! I'm ZIPPY the PINHEAD
                                  at               and I'm totally committed
                              gmail.com            to the festive mode.

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Grant Edwards]

On 2012-06-06, David Miller <davem@davemloft.net> wrote:
> From: Grant Edwards <grant.b.edwards@gmail.com>
> Date: Wed, 6 Jun 2012 19:01:40 +0000 (UTC)
>
>> That's what I'll do as soon as I can find a definition of what the API
>> for alloc_skb() actually _is_.  It has clearly changed in the past few
>> years.
>
> And it will continue to change.  There are no stable APIs inside of
> the kernel, none.

Oh, I know (as does anybody who has maintained a driver for more than
a few months).  Right now, I'm just trying to find out what the
current API for alloc_skb() is.

Is skb_tailroom() guaranteed to be >= the requested size?

-- 
Grant Edwards               grant.b.edwards        Yow! I'm wet!  I'm wild!
                                  at               
                              gmail.com            

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Grant Edwards]

On 2012-06-06, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2012-06-06 at 11:51 -0700, David Miller wrote:
>> From: Grant Edwards <grant.b.edwards@gmail.com>
>> Date: Wed, 6 Jun 2012 18:32:57 +0000 (UTC)
>> 
>> > The kernel module that's started failing fills the allocated sk_buff
>> > until tailroom() indicates it is full and then sends it.  The problem
>> > is that sending a packet with a length of 1850 won't work (it's a
>> > MAC-layer Ethernet packet).
>> 
>> The amount of tailroom an SKB has is implementation dependent.
>> 
>> It's incredibly poor form to rely upon it to determine whether a
>> fully sized frame has been constructed or not.
>> 
>> Please fix the code that does this.
>
> By the way, we had a similar problem, and the fix was :
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=a21d45726acacc963d8baddf74607d9b74e2b723
>
> Grant, depending on the context, you might use skb->avail_size and
> skb_availroom() as well.
>
> Beware skb->avail_size is unioned with skb->{mark|dropcount}

Thanks for the pointer.

-- 
Grant Edwards               grant.b.edwards        Yow! ANN JILLIAN'S HAIR
                                  at               makes LONI ANDERSON'S
                              gmail.com            HAIR look like RICARDO
                                                   MONTALBAN'S HAIR!

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Eric Dumazet]

On Wed, 2012-06-06 at 20:24 +0000, Grant Edwards wrote:

> Is skb_tailroom() guaranteed to be >= the requested size?
> 

Of course, but only right after alloc_skb().

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Grant Edwards]

On 2012-06-06, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2012-06-06 at 20:24 +0000, Grant Edwards wrote:
>
>> Is skb_tailroom() guaranteed to be >= the requested size?
>> 
>
> Of course, but only right after alloc_skb().

That's good enough.
     
-- 
Grant Edwards               grant.b.edwards        Yow! Okay ... I'm going
                                  at               home to write the "I HATE
                              gmail.com            RUBIK's CUBE HANDBOOK FOR
                                                   DEAD CAT LOVERS" ...

[PATCH net-next] net: Update kernel-doc for __alloc_skb()

[Ben Hutchings]

__alloc_skb() now extends tailroom to allow the use of padding added
by the heap allocator.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 net/core/skbuff.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 016694d..1d74cea 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -160,8 +160,8 @@ static void skb_under_panic(struct sk_buff *skb, int sz, void *here)
  *	@node: numa node to allocate memory on
  *
  *	Allocate a new &sk_buff. The returned buffer has no headroom and a
- *	tail room of size bytes. The object has a reference count of one.
- *	The return is the buffer. On a failure the return is %NULL.
+ *	tail room of at least size bytes. The object has a reference count
+ *	of one. The return is the buffer. On a failure the return is %NULL.
  *
  *	Buffers may only be allocated from interrupts using a @gfp_mask of
  *	%GFP_ATOMIC.
-- 
1.7.7.6


-- 
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.

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Grant Edwards]

On 2012-06-06, David Miller <davem@davemloft.net> wrote:
> From: Grant Edwards <grant.b.edwards@gmail.com>
> Date: Wed, 6 Jun 2012 18:59:19 +0000 (UTC)
>
>> At the time it was written (probably 10+ years ago) it was relying on
>> the documented API for alloc_skb() that stated alloc_skb() either
>> returned an sk_buff of the requested size or it failed.
>
> It was never a formal API that we would only allocate 'size'
> amount of tailroom.

How can you say that?

>From skbuff.c:

    /**
    *__alloc_skb-allocate a network buffer
    *@size: size to allocate
    *@gfp_mask: allocation mask
    *@fclone: allocate from fclone cache instead of head cache
    *and allocate a cloned (child) skb
    *@node: numa node to allocate memory on
    *
>>> *Allocate a new &sk_buff. The returned buffer has no headroom and a
>>> *tail room of size bytes. The object has a reference count of one.
    *The return is the buffer. On a failure the return is %NULL.
    *
    *Buffers may only be allocated from interrupts using a @gfp_mask of
    *%GFP_ATOMIC.
    */

-- 
Grant Edwards               grant.b.edwards        Yow! Did you move a lot of
                                  at               KOREAN STEAK KNIVES this
                              gmail.com            trip, Dingy?

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Eric Dumazet]

On Thu, 2012-06-07 at 13:23 +0000, Grant Edwards wrote:
> On 2012-06-06, David Miller <davem@davemloft.net> wrote:
> > From: Grant Edwards <grant.b.edwards@gmail.com>
> > Date: Wed, 6 Jun 2012 18:59:19 +0000 (UTC)
> >
> >> At the time it was written (probably 10+ years ago) it was relying on
> >> the documented API for alloc_skb() that stated alloc_skb() either
> >> returned an sk_buff of the requested size or it failed.
> >
> > It was never a formal API that we would only allocate 'size'
> > amount of tailroom.
> 
> How can you say that?


Documentation was stale, so what ?

kmalloc(99) doesnt allocate 99 bytes but 128, so what ?

Grant, what about you fix your code ?

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Grant Edwards]

On Thu, Jun 07, 2012 at 04:01:50PM +0200, Eric Dumazet wrote:
> On Thu, 2012-06-07 at 13:23 +0000, Grant Edwards wrote:
> > On 2012-06-06, David Miller <davem@davemloft.net> wrote:

> > > It was never a formal API that we would only allocate 'size'
> > > amount of tailroom.
> >
> > How can you say that?

> Documentation was stale, so what ?

So there _was_ a formal API that said you would only allocate 'size'
amount of tailroom.  That's what.

> kmalloc(99) doesnt allocate 99 bytes but 128, so what?

Doing so violated the documented API.

You said there was never any API definition that said tailroom() ==
requested size, and implied that it was stupid to write code that
expected tailroom() == requested size.

I was merely pointing out that the API was indeed documented that way.

> Grant, what about you fix your code ?

I did.

And the API documentation has now been fixed as well, but don't try to
tell me that the API documentation didn't promise to work the way my
code expected it to work.

-- 
Grant Edwards               grant.b.edwards        Yow! Youth of today!
                                  at               Join me in a mass rally
                              gmail.com            for traditional mental
                                                   attitudes!

Re: Change in alloc_skb() behavior in 3.2+ kernels?

[Eric Dumazet]

On Thu, 2012-06-07 at 14:16 +0000, Grant Edwards wrote:

> I was merely pointing out that the API was indeed documented that way.


Good, you are right and we were wrong.

Hopefully you can still use linux-2

Re: [PATCH net-next] net: Update kernel-doc for __alloc_skb()

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 7 Jun 2012 02:23:37 +0100

> __alloc_skb() now extends tailroom to allow the use of padding added
> by the heap allocator.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Applied.