[PATCH] xfrm: use printk instead of WARN for bad policy reporting

[PATCH] xfrm: use printk instead of WARN for bad policy reporting

[Vegard Nossum]

AFAICT this message is just printed whenever input validation fails.
This is a normal failure and we shouldn't be dumping the stack over it.

Looks like it was originally a printk that was maybe incorrectly
upgraded to a WARN:

commit 62db5cfd70b1ef53aa21f144a806fe3b78c84fab
Author: stephen hemminger <shemminger@vyatta.com>
Date:   Wed May 12 06:37:06 2010 +0000

    xfrm: add severity to printk

Cc: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 net/xfrm/xfrm_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 4fb04ce..0b81bfc 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2150,7 +2150,7 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
 	return 0;
 
 bad_policy:
-	WARN(1, "BAD policy passed\n");
+	printk(KERN_WARNING "xfrm_user: bad policy passed\n");
 free_state:
 	kfree(x);
 nomem:
-- 
1.9.1

Re: [PATCH] xfrm: use printk instead of WARN for bad policy reporting

[Vegard Nossum]

On 07/20/2016 10:32 AM, Vegard Nossum wrote:
> AFAICT this message is just printed whenever input validation fails.
> This is a normal failure and we shouldn't be dumping the stack over it.
>
> Looks like it was originally a printk that was maybe incorrectly
> upgraded to a WARN:
>
> commit 62db5cfd70b1ef53aa21f144a806fe3b78c84fab
> Author: stephen hemminger <shemminger@vyatta.com>
> Date:   Wed May 12 06:37:06 2010 +0000
>
>      xfrm: add severity to printk

Just FYI I'm also running into the

// reset the timers here?
WARN(1, "Don't know what to do with soft policy expire\n");

in xfrm_add_pol_expire() from the same commit, but that looks
potentially somewhat more serious (or at least it looks like we might
want to do some sort of cleaning up), so I won't touch it for now.

Added some more XFRM people to Cc.


Vegard

 > Cc: Stephen Hemminger <stephen@networkplumber.org>
 > Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
 > ---
 >   net/xfrm/xfrm_user.c | 2 +-
 >   1 file changed, 1 insertion(+), 1 deletion(-)
 >
 > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
 > index 4fb04ce..0b81bfc 100644
 > --- a/net/xfrm/xfrm_user.c
 > +++ b/net/xfrm/xfrm_user.c
 > @@ -2150,7 +2150,7 @@ static int xfrm_add_acquire(struct sk_buff 
*skb, struct nlmsghdr *nlh,
 >   	return 0;
 >
 >   bad_policy:
 > -	WARN(1, "BAD policy passed\n");
 > +	printk(KERN_WARNING "xfrm_user: bad policy passed\n");
 >   free_state:
 >   	kfree(x);
 >   nomem:
 >

Re: [PATCH] xfrm: use printk instead of WARN for bad policy reporting

[Steffen Klassert]

On Wed, Jul 20, 2016 at 10:32:35AM +0200, Vegard Nossum wrote:
> AFAICT this message is just printed whenever input validation fails.
> This is a normal failure and we shouldn't be dumping the stack over it.
> 
> Looks like it was originally a printk that was maybe incorrectly
> upgraded to a WARN:
> 
> commit 62db5cfd70b1ef53aa21f144a806fe3b78c84fab
> Author: stephen hemminger <shemminger@vyatta.com>
> Date:   Wed May 12 06:37:06 2010 +0000
> 
>     xfrm: add severity to printk
> 
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  net/xfrm/xfrm_user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 4fb04ce..0b81bfc 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -2150,7 +2150,7 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	return 0;
>  
>  bad_policy:
> -	WARN(1, "BAD policy passed\n");
> +	printk(KERN_WARNING "xfrm_user: bad policy passed\n");

Why should we print here anything at all? If it is a normal
configuration error, we should remove the printing.

Re: [PATCH] xfrm: use printk instead of WARN for bad policy reporting

[Herbert Xu]

On Wed, Jul 20, 2016 at 01:53:12PM +0200, Vegard Nossum wrote:
> On 07/20/2016 10:32 AM, Vegard Nossum wrote:
> >AFAICT this message is just printed whenever input validation fails.
> >This is a normal failure and we shouldn't be dumping the stack over it.
> >
> >Looks like it was originally a printk that was maybe incorrectly
> >upgraded to a WARN:
> >
> >commit 62db5cfd70b1ef53aa21f144a806fe3b78c84fab
> >Author: stephen hemminger <shemminger@vyatta.com>
> >Date:   Wed May 12 06:37:06 2010 +0000
> >
> >     xfrm: add severity to printk
> 
> Just FYI I'm also running into the
> 
> // reset the timers here?
> WARN(1, "Don't know what to do with soft policy expire\n");
> 
> in xfrm_add_pol_expire() from the same commit, but that looks
> potentially somewhat more serious (or at least it looks like we might
> want to do some sort of cleaning up), so I won't touch it for now.

It certainly shouldn't be a WARN, it probably shouldn't print
anything either.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] xfrm: use printk instead of WARN for bad policy reporting

[Vegard Nossum]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

On 07/20/2016 02:15 PM, Steffen Klassert wrote:
> On Wed, Jul 20, 2016 at 10:32:35AM +0200, Vegard Nossum wrote:
>> AFAICT this message is just printed whenever input validation fails.
>> This is a normal failure and we shouldn't be dumping the stack over it.
>>
>> Looks like it was originally a printk that was maybe incorrectly
>> upgraded to a WARN:
>>
>> commit 62db5cfd70b1ef53aa21f144a806fe3b78c84fab
>> Author: stephen hemminger <shemminger@vyatta.com>
>> Date:   Wed May 12 06:37:06 2010 +0000
>>
>>      xfrm: add severity to printk
>>
>> Cc: Stephen Hemminger <stephen@networkplumber.org>
>> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>> ---
>>   net/xfrm/xfrm_user.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
>> index 4fb04ce..0b81bfc 100644
>> --- a/net/xfrm/xfrm_user.c
>> +++ b/net/xfrm/xfrm_user.c
>> @@ -2150,7 +2150,7 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
>>   	return 0;
>>
>>   bad_policy:
>> -	WARN(1, "BAD policy passed\n");
>> +	printk(KERN_WARNING "xfrm_user: bad policy passed\n");
>
> Why should we print here anything at all? If it is a normal
> configuration error, we should remove the printing.
>

New patch attached. Thanks,


Vegard

[-- Attachment #2: 0001-xfrm-get-rid-of-incorrect-WARN.patch --]
[-- Type: text/x-patch, Size: 1466 bytes --]

>From 5bc56901bfea60e8d521f859ce73180796df0e56 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@oracle.com>
Date: Tue, 19 Jul 2016 23:41:16 -0700
Subject: [PATCH] xfrm: get rid of incorrect WARN

AFAICT this message is just printed whenever input validation fails.
This is a normal failure and we shouldn't be dumping the stack over it.

Looks like it was originally a printk that was maybe incorrectly
upgraded to a WARN:

commit 62db5cfd70b1ef53aa21f144a806fe3b78c84fab
Author: stephen hemminger <shemminger@vyatta.com>
Date:   Wed May 12 06:37:06 2010 +0000

    xfrm: add severity to printk

Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 net/xfrm/xfrm_user.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index d516845..2477b24 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2117,7 +2117,7 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	err = verify_newpolicy_info(&ua->policy);
 	if (err)
-		goto bad_policy;
+		goto free_state;
 
 	/*   build an XP */
 	xp = xfrm_policy_construct(net, &ua->policy, attrs, &err);
@@ -2149,8 +2149,6 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	return 0;
 
-bad_policy:
-	WARN(1, "BAD policy passed\n");
 free_state:
 	kfree(x);
 nomem:
-- 
1.9.1

Re: [PATCH] xfrm: use printk instead of WARN for bad policy reporting

[Vegard Nossum]

[-- Attachment #1: Type: text/plain, Size: 832 bytes --]

On 07/27/2016 05:01 AM, Herbert Xu wrote:
> On Wed, Jul 20, 2016 at 01:53:12PM +0200, Vegard Nossum wrote:
>> Just FYI I'm also running into the
>>
>> // reset the timers here?
>> WARN(1, "Don't know what to do with soft policy expire\n");
>>
>> in xfrm_add_pol_expire() from the same commit, but that looks
>> potentially somewhat more serious (or at least it looks like we might
>> want to do some sort of cleaning up), so I won't touch it for now.
>
> It certainly shouldn't be a WARN, it probably shouldn't print
> anything either.

Here's another patch to remove that too.

I don't actually *use* this code myself and I feel the justification
I've given for removing the WARN to be a bit weak, so if you don't take
the patch I'll just keep it in my local tree to keep it from showing up
again during fuzzing.

Thanks,


Vegard

[-- Attachment #2: 0001-xfrm-get-rid-of-another-incorrect-WARN.patch --]
[-- Type: text/x-patch, Size: 1099 bytes --]

>From 5b302eb4c188064a69176a901c2bec3e19440c03 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@oracle.com>
Date: Wed, 27 Jul 2016 08:13:14 +0200
Subject: [PATCH] xfrm: get rid of another incorrect WARN

During fuzzing I regularly run into this WARN(). According to Herbert Xu,
this "certainly shouldn't be a WARN, it probably shouldn't print anything
either".

Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 net/xfrm/xfrm_user.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 2477b24..a4e44f7 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2053,7 +2053,6 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 		xfrm_audit_policy_delete(xp, 1, true);
 	} else {
 		// reset the timers here?
-		WARN(1, "Don't know what to do with soft policy expire\n");
 	}
 	km_policy_expired(xp, p->dir, up->hard, nlh->nlmsg_pid);
 
-- 
1.9.1

Re: [PATCH] xfrm: use printk instead of WARN for bad policy reporting

[Herbert Xu]

On Wed, Jul 27, 2016 at 08:20:57AM +0200, Vegard Nossum wrote:
>
> Here's another patch to remove that too.
> 
> I don't actually *use* this code myself and I feel the justification
> I've given for removing the WARN to be a bit weak, so if you don't take
> the patch I'll just keep it in my local tree to keep it from showing up
> again during fuzzing.

Please just kill the whole else clause.  For soft policy expires
we simply need to relay a message to the KM and nothing more.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] xfrm: use printk instead of WARN for bad policy reporting

[Vegard Nossum]

[-- Attachment #1: Type: text/plain, Size: 558 bytes --]

On 07/27/2016 08:31 AM, Herbert Xu wrote:
> On Wed, Jul 27, 2016 at 08:20:57AM +0200, Vegard Nossum wrote:
>>
>> Here's another patch to remove that too.
>>
>> I don't actually *use* this code myself and I feel the justification
>> I've given for removing the WARN to be a bit weak, so if you don't take
>> the patch I'll just keep it in my local tree to keep it from showing up
>> again during fuzzing.
>
> Please just kill the whole else clause.  For soft policy expires
> we simply need to relay a message to the KM and nothing more.

Try #2 :-)


Vegard

[-- Attachment #2: 0001-xfrm-get-rid-of-another-incorrect-WARN.patch --]
[-- Type: text/x-patch, Size: 1155 bytes --]

>From e5111e4dcd0e0c0990d3f4bba0ba0bc9d0b40bae Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@oracle.com>
Date: Wed, 27 Jul 2016 08:13:14 +0200
Subject: [PATCH] xfrm: get rid of another incorrect WARN

During fuzzing I regularly run into this WARN(). According to Herbert Xu,
this "certainly shouldn't be a WARN, it probably shouldn't print anything
either".

Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 net/xfrm/xfrm_user.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 2477b24..10e4e26 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2051,9 +2051,6 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (up->hard) {
 		xfrm_policy_delete(xp, p->dir);
 		xfrm_audit_policy_delete(xp, 1, true);
-	} else {
-		// reset the timers here?
-		WARN(1, "Don't know what to do with soft policy expire\n");
 	}
 	km_policy_expired(xp, p->dir, up->hard, nlh->nlmsg_pid);
 
-- 
1.9.1

Re: [PATCH] xfrm: use printk instead of WARN for bad policy reporting

[Steffen Klassert]

On Wed, Jul 27, 2016 at 08:03:18AM +0200, Vegard Nossum wrote:
> Subject: [PATCH] xfrm: get rid of incorrect WARN
> 
> AFAICT this message is just printed whenever input validation fails.
> This is a normal failure and we shouldn't be dumping the stack over it.
> 
> Looks like it was originally a printk that was maybe incorrectly
> upgraded to a WARN:
> 
> commit 62db5cfd70b1ef53aa21f144a806fe3b78c84fab
> Author: stephen hemminger <shemminger@vyatta.com>
> Date:   Wed May 12 06:37:06 2010 +0000
> 
>     xfrm: add severity to printk
> 
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

Applied to the ipsec tree, thanks!

Re: [PATCH] xfrm: use printk instead of WARN for bad policy reporting

[Steffen Klassert]

On Wed, Jul 27, 2016 at 08:44:15AM +0200, Vegard Nossum wrote:
> On 07/27/2016 08:31 AM, Herbert Xu wrote:
> >On Wed, Jul 27, 2016 at 08:20:57AM +0200, Vegard Nossum wrote:
> >>
> >>Here's another patch to remove that too.
> >>
> >>I don't actually *use* this code myself and I feel the justification
> >>I've given for removing the WARN to be a bit weak, so if you don't take
> >>the patch I'll just keep it in my local tree to keep it from showing up
> >>again during fuzzing.
> >
> >Please just kill the whole else clause.  For soft policy expires
> >we simply need to relay a message to the KM and nothing more.
> 
> Try #2 :-)
> 
> 
> Vegard

> >From e5111e4dcd0e0c0990d3f4bba0ba0bc9d0b40bae Mon Sep 17 00:00:00 2001
> From: Vegard Nossum <vegard.nossum@oracle.com>
> Date: Wed, 27 Jul 2016 08:13:14 +0200
> Subject: [PATCH] xfrm: get rid of another incorrect WARN
> 
> During fuzzing I regularly run into this WARN(). According to Herbert Xu,
> this "certainly shouldn't be a WARN, it probably shouldn't print anything
> either".
> 
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

Also applied to the ipsec tree, thanks a lot!