[PATCH] net: skbuff: use _RET_IP_

[PATCH] net: skbuff: use _RET_IP_

[Davidlohr Bueso]

Use the already defined macro to pass the function return address.

Signed-off-by: Davidlohr Bueso <davidlohr.bueso@hp.com>
---
 net/core/skbuff.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index af9185d..0d06850 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -116,22 +116,22 @@ static const struct pipe_buf_operations sock_pipe_buf_ops = {
  *	Keep out of line to prevent kernel bloat.
  *	__builtin_return_address is not used because it is not always reliable.
  */
-static void skb_panic(struct sk_buff *skb, unsigned int sz, void *addr,
+static void skb_panic(struct sk_buff *skb, unsigned int sz, unsigned long addr,
 		      const char msg[])
 {
-	pr_emerg("%s: text:%p len:%d put:%d head:%p data:%p tail:%#lx end:%#lx dev:%s\n",
+	pr_emerg("%s: text:0x%lx len:%d put:%d head:%p data:%p tail:%#lx end:%#lx dev:%s\n",
 		 msg, addr, skb->len, sz, skb->head, skb->data,
 		 (unsigned long)skb->tail, (unsigned long)skb->end,
 		 skb->dev ? skb->dev->name : "<NULL>");
 	BUG();
 }
 
-static void skb_over_panic(struct sk_buff *skb, unsigned int sz, void *addr)
+static void skb_over_panic(struct sk_buff *skb, unsigned int sz, unsigned long addr)
 {
 	skb_panic(skb, sz, addr, __func__);
 }
 
-static void skb_under_panic(struct sk_buff *skb, unsigned int sz, void *addr)
+static void skb_under_panic(struct sk_buff *skb, unsigned int sz, unsigned long addr)
 {
 	skb_panic(skb, sz, addr, __func__);
 }
@@ -646,7 +646,7 @@ void kfree_skb(struct sk_buff *skb)
 		smp_rmb();
 	else if (likely(!atomic_dec_and_test(&skb->users)))
 		return;
-	trace_kfree_skb(skb, __builtin_return_address(0));
+	trace_kfree_skb(skb, (void *) _RET_IP_);
 	__kfree_skb(skb);
 }
 EXPORT_SYMBOL(kfree_skb);
@@ -1279,7 +1279,7 @@ unsigned char *skb_put(struct sk_buff *skb, unsigned int len)
 	skb->tail += len;
 	skb->len  += len;
 	if (unlikely(skb->tail > skb->end))
-		skb_over_panic(skb, len, __builtin_return_address(0));
+		skb_over_panic(skb, len, _RET_IP_);
 	return tmp;
 }
 EXPORT_SYMBOL(skb_put);
@@ -1298,7 +1298,7 @@ unsigned char *skb_push(struct sk_buff *skb, unsigned int len)
 	skb->data -= len;
 	skb->len  += len;
 	if (unlikely(skb->data<skb->head))
-		skb_under_panic(skb, len, __builtin_return_address(0));
+		skb_under_panic(skb, len, _RET_IP_);
 	return skb->data;
 }
 EXPORT_SYMBOL(skb_push);
-- 
1.7.11.7

Re: [PATCH] net: skbuff: use _RET_IP_

[Sergei Shtylyov]

Hello.

On 05/30/2013 01:09 AM, Davidlohr Bueso wrote:

> Use the already defined macro to pass the function return address.
>
> Signed-off-by: Davidlohr Bueso <davidlohr.bueso@hp.com>
> ---
>   net/core/skbuff.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index af9185d..0d06850 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -116,22 +116,22 @@ static const struct pipe_buf_operations sock_pipe_buf_ops = {
>    *	Keep out of line to prevent kernel bloat.
>    *	__builtin_return_address is not used because it is not always reliable.
>    */
> -static void skb_panic(struct sk_buff *skb, unsigned int sz, void *addr,
> +static void skb_panic(struct sk_buff *skb, unsigned int sz, unsigned long addr,
>   		      const char msg[])
>   {
> -	pr_emerg("%s: text:%p len:%d put:%d head:%p data:%p tail:%#lx end:%#lx dev:%s\n",
> +	pr_emerg("%s: text:0x%lx len:%d put:%d head:%p data:%p tail:%#lx end:%#lx dev:%s\n",

     Why not "text:%#lx" as already used in this string? It's equivalent 
to "0x%lx".

WBR, Sergei

Re: [PATCH] net: skbuff: use _RET_IP_

[Bjørn Mork]

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:

>     Why not "text:%#lx" as already used in this string? It's
> equivalent to "0x%lx".

Well, I don't know the reasoning in this case, but I'd like to note that
those are not strictly equivalent.  Personally I find the formatting of 0
annoying enough to avoid %#x for any value which may be 0.  It's
especially bad if you try to line up things by adding leading zeros.

I would expect these to produce the same result, but they don't:

	printf("0x%02hhx\n", 0);
	printf("%#04hhx\n", 0);

Ending up with a 4 digit output when you expect 2 is very confusing.  It
doesn't matter that 0 is 0 in any case.  Why doesn't the same happen to
1 then?  This is just inconsistent behaviour, and I see no valid excuse
for it.

IMHO the single format character saved isn't worth this at all.  I'll
continue using 0x%x


Bjørn

Re: [PATCH] net: skbuff: use _RET_IP_

[Davidlohr Bueso]

On Thu, 2013-05-30 at 13:08 +0200, Bjørn Mork wrote:
> Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:
> 
> >     Why not "text:%#lx" as already used in this string? It's
> > equivalent to "0x%lx".
> 
> Well, I don't know the reasoning in this case, but I'd like to note that
> those are not strictly equivalent.  Personally I find the formatting of 0
> annoying enough to avoid %#x for any value which may be 0.  It's
> especially bad if you try to line up things by adding leading zeros.

Yep, I found that 0x%lx produced the same output as %p. If you'd rather
I use something else instead, I'm happy to update the patch.

Thanks,
Davidlohr

Re: [PATCH] net: skbuff: use _RET_IP_

[Joe Perches]

On Thu, 2013-05-30 at 18:11 -0700, Davidlohr Bueso wrote:
> On Thu, 2013-05-30 at 13:08 +0200, Bjørn Mork wrote:
> > Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:
> > 
> > >     Why not "text:%#lx" as already used in this string? It's
> > > equivalent to "0x%lx".
> > 
> > Well, I don't know the reasoning in this case, but I'd like to note that
> > those are not strictly equivalent.  Personally I find the formatting of 0
> > annoying enough to avoid %#x for any value which may be 0.  It's
> > especially bad if you try to line up things by adding leading zeros.
> 
> Yep, I found that 0x%lx produced the same output as %p.

Don't use a standalone gcc compiled program to
determine what the kernel outputs.

lib/vsprintf.c does not output the same. (32 bit)

The kernel output is;

	printk("0x%lx\n", 0x100ul)		0x100
	printk("%p\n", (void *)0x100ul)		00000100
	printk("%#p\n", (void *)0x100ul)	0x00000100

The last one isn't used at all in kernel source. (gcc complains)
It's always "0x%p"

Re: [PATCH] net: skbuff: use _RET_IP_

[Bjørn Mork]

Joe Perches <joe@perches.com> writes:
> On Thu, 2013-05-30 at 18:11 -0700, Davidlohr Bueso wrote:
>> On Thu, 2013-05-30 at 13:08 +0200, Bjørn Mork wrote:
>> > Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:
>> > 
>> > >     Why not "text:%#lx" as already used in this string? It's
>> > > equivalent to "0x%lx".
>> > 
>> > Well, I don't know the reasoning in this case, but I'd like to note that
>> > those are not strictly equivalent.  Personally I find the formatting of 0
>> > annoying enough to avoid %#x for any value which may be 0.  It's
>> > especially bad if you try to line up things by adding leading zeros.
>> 
>> Yep, I found that 0x%lx produced the same output as %p.
>
> Don't use a standalone gcc compiled program to
> determine what the kernel outputs.

That's a very good point.  Sorry for mixing this up.  You are of course
right.  The kernel does everything correctly, so there is no reason not
to use %#x



Bjørn

Re: [PATCH] net: skbuff: use _RET_IP_

[Sergei Shtylyov]

Hello.

On 31-05-2013 9:20, Joe Perches wrote:

>>>>      Why not "text:%#lx" as already used in this string? It's
>>>> equivalent to "0x%lx".

>>> Well, I don't know the reasoning in this case, but I'd like to note that
>>> those are not strictly equivalent.  Personally I find the formatting of 0
>>> annoying enough to avoid %#x for any value which may be 0.  It's
>>> especially bad if you try to line up things by adding leading zeros.

>> Yep, I found that 0x%lx produced the same output as %p.

> Don't use a standalone gcc compiled program to
> determine what the kernel outputs.

> lib/vsprintf.c does not output the same. (32 bit)

> The kernel output is;

> 	printk("0x%lx\n", 0x100ul)		0x100
> 	printk("%p\n", (void *)0x100ul)		00000100
> 	printk("%#p\n", (void *)0x100ul)	0x00000100

> The last one isn't used at all in kernel source. (gcc complains)
> It's always "0x%p"

     I was talking about using "%#lx", not "%#p". I don't see it in your 
example.

WBR, Sergei

Re: [PATCH] net: skbuff: use _RET_IP_

[Joe Perches]

On Fri, 2013-05-31 at 18:33 +0400, Sergei Shtylyov wrote:
> On 31-05-2013 9:20, Joe Perches wrote:
> > Don't use a standalone gcc compiled program to
> > determine what the kernel outputs.
[]
> > The kernel output is;
> 
> > 	printk("0x%lx\n", 0x100ul)		0x100
> > 	printk("%p\n", (void *)0x100ul)		00000100
> > 	printk("%#p\n", (void *)0x100ul)	0x00000100
> 
> > The last one isn't used at all in kernel source. (gcc complains)
> > It's always "0x%p"
> 
>      I was talking about using "%#lx", not "%#p". I don't see it in your 
> example.

"0x%lx" and "%#lx" produce the same output in the kernel.

The latter isn't used very often though.
I expect most coders don't know it exists/works.

$ git grep -E "0x%l{0,2}x" | wc -l
12542
$ git grep -E "%#l{0,2}x" | wc -l
1737

(some false positives there of course)

Also, some might expect that "%#08lx", is 10 chars wide,
but it's only 8, so maybe "0x%08lx" is better used.

The "%#08lx" width defect seems pretty common:

$ git grep -E -i "%#0(8|16)l{0,2}x" | wc -l
253

Re: [PATCH] net: skbuff: use _RET_IP_

[Sergei Shtylyov]

Hello.

On 05/31/2013 08:54 PM, Joe Perches wrote:

>>> Don't use a standalone gcc compiled program to
>>> determine what the kernel outputs.
> []
>>> The kernel output is;
>>> 	printk("0x%lx\n", 0x100ul)		0x100
>>> 	printk("%p\n", (void *)0x100ul)		00000100
>>> 	printk("%#p\n", (void *)0x100ul)	0x00000100
>>> The last one isn't used at all in kernel source. (gcc complains)
>>> It's always "0x%p"
>>       I was talking about using "%#lx", not "%#p". I don't see it in your
>> example.
> "0x%lx" and "%#lx" produce the same output in the kernel.
>
> The latter isn't used very often though.

    It's already used in the same format string in this case. That's why 
I suggested
not to deviate from the existing code.

WBR, Sergei

Re: [PATCH] net: skbuff: use _RET_IP_

[David Miller]

From: Davidlohr Bueso <davidlohr.bueso@hp.com>
Date: Wed, 29 May 2013 14:09:03 -0700

> Use the already defined macro to pass the function return address.
> 
> Signed-off-by: Davidlohr Bueso <davidlohr.bueso@hp.com>

I've decided that we should just leave this alone for now, because
frankly the choice is arbitrary.