linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] flow_dissector: avoid uninitialized variable access
@ 2016-10-21 15:55 Arnd Bergmann
  2016-10-21 16:31 ` Jiri Pirko
  0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2016-10-21 15:55 UTC (permalink / raw)
  To: David S. Miller
  Cc: Arnd Bergmann, Alexander Duyck, Tom Herbert, Jiri Pirko,
	Hadar Hen Zion, Gao Feng, Eric Garver, Amir Vadai, netdev,
	linux-kernel

gcc warns about an uninitialized pointer dereference in the vlan
priority handling:

net/core/flow_dissector.c: In function '__skb_flow_dissect':
net/core/flow_dissector.c:281:61: error: 'vlan' may be used uninitialized in this function [-Werror=maybe-uninitialized]

>From all I can tell, this warning is about a real bug, and we
should not attempt look up the vlan header if there was
no vlan tag.

Fixes: f6a66927692e ("flow_dissector: Get vlan priority in addition to vlan id")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I'm not sure about this one, please have a closer look at what
the original code does before applying.
---
 net/core/flow_dissector.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 44e6ba9d3a6b..dd6003bf27e1 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -245,7 +245,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 	}
 	case htons(ETH_P_8021AD):
 	case htons(ETH_P_8021Q): {
-		const struct vlan_hdr *vlan;
+		const struct vlan_hdr *vlan = NULL;
 
 		if (skb && skb_vlan_tag_present(skb))
 			proto = skb->protocol;
@@ -264,7 +264,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 		}
 
 		skip_vlan = true;
-		if (dissector_uses_key(flow_dissector,
+		if (vlan && dissector_uses_key(flow_dissector,
 				       FLOW_DISSECTOR_KEY_VLAN)) {
 			key_vlan = skb_flow_dissector_target(flow_dissector,
 							     FLOW_DISSECTOR_KEY_VLAN,
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH] flow_dissector: avoid uninitialized variable access
  2016-10-21 15:55 [PATCH] flow_dissector: avoid uninitialized variable access Arnd Bergmann
@ 2016-10-21 16:31 ` Jiri Pirko
  2016-10-21 21:05   ` Arnd Bergmann
  2016-10-22  1:48   ` [PATCH] flow_dissector: avoid uninitialized variable access Linus Torvalds
  0 siblings, 2 replies; 11+ messages in thread
From: Jiri Pirko @ 2016-10-21 16:31 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: David S. Miller, Alexander Duyck, Tom Herbert, Jiri Pirko,
	Hadar Hen Zion, Gao Feng, Eric Garver, Amir Vadai, netdev,
	linux-kernel

Fri, Oct 21, 2016 at 05:55:53PM CEST, arnd@arndb.de wrote:
>gcc warns about an uninitialized pointer dereference in the vlan
>priority handling:
>
>net/core/flow_dissector.c: In function '__skb_flow_dissect':
>net/core/flow_dissector.c:281:61: error: 'vlan' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
>From all I can tell, this warning is about a real bug, and we
>should not attempt look up the vlan header if there was
>no vlan tag.

I don't see how vlan could be used uninitialized. But I understand that
this is impossible for gcc to track it. Please just use uninitialized_var()



>
>Fixes: f6a66927692e ("flow_dissector: Get vlan priority in addition to vlan id")
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>---
>I'm not sure about this one, please have a closer look at what
>the original code does before applying.
>---
> net/core/flow_dissector.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 44e6ba9d3a6b..dd6003bf27e1 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -245,7 +245,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 	}
> 	case htons(ETH_P_8021AD):
> 	case htons(ETH_P_8021Q): {
>-		const struct vlan_hdr *vlan;
>+		const struct vlan_hdr *vlan = NULL;
> 
> 		if (skb && skb_vlan_tag_present(skb))
> 			proto = skb->protocol;
>@@ -264,7 +264,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 		}
> 
> 		skip_vlan = true;
>-		if (dissector_uses_key(flow_dissector,
>+		if (vlan && dissector_uses_key(flow_dissector,
> 				       FLOW_DISSECTOR_KEY_VLAN)) {
> 			key_vlan = skb_flow_dissector_target(flow_dissector,
> 							     FLOW_DISSECTOR_KEY_VLAN,
>-- 
>2.9.0
>

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] flow_dissector: avoid uninitialized variable access
  2016-10-21 16:31 ` Jiri Pirko
@ 2016-10-21 21:05   ` Arnd Bergmann
  2016-10-21 22:16     ` Arnd Bergmann
  2016-10-22  1:48   ` [PATCH] flow_dissector: avoid uninitialized variable access Linus Torvalds
  1 sibling, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2016-10-21 21:05 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: David S. Miller, Alexander Duyck, Tom Herbert, Jiri Pirko,
	Hadar Hen Zion, Gao Feng, Eric Garver, Amir Vadai, netdev,
	linux-kernel

On Friday, October 21, 2016 6:31:18 PM CEST Jiri Pirko wrote:
> Fri, Oct 21, 2016 at 05:55:53PM CEST, arnd@arndb.de wrote:
> >gcc warns about an uninitialized pointer dereference in the vlan
> >priority handling:
> >
> >net/core/flow_dissector.c: In function '__skb_flow_dissect':
> >net/core/flow_dissector.c:281:61: error: 'vlan' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >
> >From all I can tell, this warning is about a real bug, and we
> >should not attempt look up the vlan header if there was
> >no vlan tag.
> 
> I don't see how vlan could be used uninitialized. But I understand that
> this is impossible for gcc to track it. Please just use uninitialized_var()
> 

I usually try to avoid uninitialized_var(), as making it obvious to
the compiler why something is known tends to result in more readable
source code and better object code.

Can you explain why "dissector_uses_key(flow_dissector,
FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
"eth_type_vlan(proto))"?

If I add uninitialized_var() here, I would at least put that in
a comment here.

On a related note, I also don't see how
"dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
implies that skb is non-NULL. I guess this is related to the
first one.

	Arnd

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] flow_dissector: avoid uninitialized variable access
  2016-10-21 21:05   ` Arnd Bergmann
@ 2016-10-21 22:16     ` Arnd Bergmann
  2016-10-22 15:57       ` Eric Garver
  0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2016-10-21 22:16 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: David S. Miller, Alexander Duyck, Tom Herbert, Jiri Pirko,
	Hadar Hen Zion, Gao Feng, Eric Garver, Amir Vadai, netdev,
	linux-kernel

On Friday, October 21, 2016 11:05:45 PM CEST Arnd Bergmann wrote:
> 
> Can you explain why "dissector_uses_key(flow_dissector,
> FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
> "eth_type_vlan(proto))"?
> 
> If I add uninitialized_var() here, I would at least put that in
> a comment here.

Found it now myself: if skb_vlan_tag_present(skb), then we don't
access 'vlan', otherwise we know it is initialized because
eth_type_vlan(proto) has to be true.
 
> On a related note, I also don't see how
> "dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
> implies that skb is non-NULL. I guess this is related to the
> first one.

I'm still unsure about this one.

I also found something else that is suspicious: 'vlan' points
to the local _vlan variable, but that has gone out of scope
by the time we access the pointer, which doesn't seem safe.

	Arnd

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] flow_dissector: avoid uninitialized variable access
  2016-10-21 16:31 ` Jiri Pirko
  2016-10-21 21:05   ` Arnd Bergmann
@ 2016-10-22  1:48   ` Linus Torvalds
  2016-10-22  6:55     ` Jiri Pirko
  1 sibling, 1 reply; 11+ messages in thread
From: Linus Torvalds @ 2016-10-22  1:48 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Arnd Bergmann, David S. Miller, Alexander Duyck, Tom Herbert,
	Jiri Pirko, Hadar Hen Zion, Gao Feng, Eric Garver, Amir Vadai,
	Network Development, Linux Kernel Mailing List

On Fri, Oct 21, 2016 at 9:31 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>
> I don't see how vlan could be used uninitialized. But I understand that
> this is impossible for gcc to track it. Please just use uninitialized_var()

Actually, I think we should never use "uninitialized_var()" except
possibly for arrays or structures that gcc can complain about.

It's a horrible thing to use, in that it adds extra cruft to the
source code, and then shuts up a compiler warning (even the _reliable_
warnings from gcc).

It's much better to just initialize the variable, and if gcc some day
gets smarter and sees that it  is unnecessary and always overwritten,
so much the better. The cost of initializing a single word is
basically zero.

            Linus

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] flow_dissector: avoid uninitialized variable access
  2016-10-22  1:48   ` [PATCH] flow_dissector: avoid uninitialized variable access Linus Torvalds
@ 2016-10-22  6:55     ` Jiri Pirko
  0 siblings, 0 replies; 11+ messages in thread
From: Jiri Pirko @ 2016-10-22  6:55 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Arnd Bergmann, David S. Miller, Alexander Duyck, Tom Herbert,
	Jiri Pirko, Hadar Hen Zion, Gao Feng, Eric Garver, Amir Vadai,
	Network Development, Linux Kernel Mailing List

Sat, Oct 22, 2016 at 03:48:48AM CEST, torvalds@linux-foundation.org wrote:
>On Fri, Oct 21, 2016 at 9:31 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> I don't see how vlan could be used uninitialized. But I understand that
>> this is impossible for gcc to track it. Please just use uninitialized_var()
>
>Actually, I think we should never use "uninitialized_var()" except
>possibly for arrays or structures that gcc can complain about.
>
>It's a horrible thing to use, in that it adds extra cruft to the
>source code, and then shuts up a compiler warning (even the _reliable_
>warnings from gcc).
>
>It's much better to just initialize the variable, and if gcc some day
>gets smarter and sees that it  is unnecessary and always overwritten,
>so much the better. The cost of initializing a single word is
>basically zero.

On the other hand, I would agrue that initializing a var to "some" value
that is never used might confuse the reader. He would naturally try to
understand the reason for that exact value in initialization.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] flow_dissector: avoid uninitialized variable access
  2016-10-21 22:16     ` Arnd Bergmann
@ 2016-10-22 15:57       ` Eric Garver
  2016-10-22 18:20         ` Tom Herbert
  0 siblings, 1 reply; 11+ messages in thread
From: Eric Garver @ 2016-10-22 15:57 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Jiri Pirko, David S. Miller, Alexander Duyck, Tom Herbert,
	Jiri Pirko, Hadar Hen Zion, Gao Feng, Amir Vadai, netdev,
	linux-kernel

On Sat, Oct 22, 2016 at 12:16:29AM +0200, Arnd Bergmann wrote:
> On Friday, October 21, 2016 11:05:45 PM CEST Arnd Bergmann wrote:
> > 
> > Can you explain why "dissector_uses_key(flow_dissector,
> > FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
> > "eth_type_vlan(proto))"?
> > 
> > If I add uninitialized_var() here, I would at least put that in
> > a comment here.
> 
> Found it now myself: if skb_vlan_tag_present(skb), then we don't
> access 'vlan', otherwise we know it is initialized because
> eth_type_vlan(proto) has to be true.
>  
> > On a related note, I also don't see how
> > "dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
> > implies that skb is non-NULL. I guess this is related to the
> > first one.
> 
> I'm still unsure about this one.

Only skb_flow_dissect_flow_keys_buf() calls this function with skb ==
NULL. It uses flow_keys_buf_dissector_keys which does not specify
FLOW_DISSECTOR_KEY_VLAN, so the if statement is false.

A similar assumption is made for FLOW_DISSECTOR_KEY_ETH_ADDRS higher up.

> I also found something else that is suspicious: 'vlan' points
> to the local _vlan variable, but that has gone out of scope
> by the time we access the pointer, which doesn't seem safe.

I see no harm in moving _vlan to the same scope as vlan.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] flow_dissector: avoid uninitialized variable access
  2016-10-22 15:57       ` Eric Garver
@ 2016-10-22 18:20         ` Tom Herbert
  2016-10-22 20:30           ` [PATCH net-next] flow_dissector: fix vlan tag handling Arnd Bergmann
  0 siblings, 1 reply; 11+ messages in thread
From: Tom Herbert @ 2016-10-22 18:20 UTC (permalink / raw)
  To: Arnd Bergmann, Jiri Pirko, David S. Miller, Alexander Duyck,
	Tom Herbert, Jiri Pirko, Hadar Hen Zion, Gao Feng, Amir Vadai,
	Linux Kernel Network Developers, LKML

On Sat, Oct 22, 2016 at 8:57 AM, Eric Garver <e@erig.me> wrote:
> On Sat, Oct 22, 2016 at 12:16:29AM +0200, Arnd Bergmann wrote:
>> On Friday, October 21, 2016 11:05:45 PM CEST Arnd Bergmann wrote:
>> >
>> > Can you explain why "dissector_uses_key(flow_dissector,
>> > FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
>> > "eth_type_vlan(proto))"?
>> >
>> > If I add uninitialized_var() here, I would at least put that in
>> > a comment here.
>>
>> Found it now myself: if skb_vlan_tag_present(skb), then we don't
>> access 'vlan', otherwise we know it is initialized because
>> eth_type_vlan(proto) has to be true.
>>
>> > On a related note, I also don't see how
>> > "dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
>> > implies that skb is non-NULL. I guess this is related to the
>> > first one.
>>
>> I'm still unsure about this one.
>
> Only skb_flow_dissect_flow_keys_buf() calls this function with skb ==
> NULL. It uses flow_keys_buf_dissector_keys which does not specify
> FLOW_DISSECTOR_KEY_VLAN, so the if statement is false.
>
> A similar assumption is made for FLOW_DISSECTOR_KEY_ETH_ADDRS higher up.
>
This is a serious problem. We can't rely on the callers to know which
keys they are allowed to use to avoid crashing the kernel. We should
fix those to check if skb is NULL, add a comment to the head of the
function warning people to never assume skb is non-NULL, and also
maybe add a degenerative check that both data argument and skb are not
NULL.

Tom

>> I also found something else that is suspicious: 'vlan' points
>> to the local _vlan variable, but that has gone out of scope
>> by the time we access the pointer, which doesn't seem safe.
>
> I see no harm in moving _vlan to the same scope as vlan.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH net-next] flow_dissector: fix vlan tag handling
  2016-10-22 18:20         ` Tom Herbert
@ 2016-10-22 20:30           ` Arnd Bergmann
  2016-10-24  8:17             ` Jiri Pirko
  0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2016-10-22 20:30 UTC (permalink / raw)
  To: Tom Herbert
  Cc: Jiri Pirko, David S. Miller, Alexander Duyck, Jiri Pirko,
	Hadar Hen Zion, Gao Feng, Amir Vadai,
	Linux Kernel Network Developers, LKML, Linus Torvalds

gcc warns about an uninitialized pointer dereference in the vlan
priority handling:

net/core/flow_dissector.c: In function '__skb_flow_dissect':
net/core/flow_dissector.c:281:61: error: 'vlan' may be used uninitialized in this function [-Werror=maybe-uninitialized]

As pointed out by Jiri Pirko, the variable is never actually used
without being initialized first as the only way it end up uninitialized
is with skb_vlan_tag_present(skb)==true, and that means it does not
get accessed.

However, the warning hints at some related issues that I'm addressing
here:

- the second check for the vlan tag is different from the first one
  that tests the skb for being NULL first, causing both the warning
  and a possible NULL pointer dereference that was not entirely fixed.
- The same patch that introduced the NULL pointer check dropped an
  earlier optimization that skipped the repeated check of the
  protocol type
- The local '_vlan' variable is referenced through the 'vlan' pointer
  but the variable has gone out of scope by the time that it is
  accessed, causing undefined behavior as the stack may have been
  overwritten.

Caching the result of the 'skb && skb_vlan_tag_present(skb)' check
in a local variable allows the compiler to further optimize the
later check. With those changes, the warning also disappears.

Fixes: 3805a938a6c2 ("flow_dissector: Check skb for VLAN only if skb specified.")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 44e6ba9d3a6b..17be1b66cc41 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -246,13 +246,10 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 	case htons(ETH_P_8021AD):
 	case htons(ETH_P_8021Q): {
 		const struct vlan_hdr *vlan;
+		struct vlan_hdr _vlan;
+		bool vlan_tag_present = (skb && skb_vlan_tag_present(skb));
 
-		if (skb && skb_vlan_tag_present(skb))
-			proto = skb->protocol;
-
-		if (eth_type_vlan(proto)) {
-			struct vlan_hdr _vlan;
-
+		if (!vlan_tag_present || eth_type_vlan(skb->protocol)) {
 			vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan),
 						    data, hlen, &_vlan);
 			if (!vlan)
@@ -270,7 +267,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 							     FLOW_DISSECTOR_KEY_VLAN,
 							     target_container);
 
-			if (skb_vlan_tag_present(skb)) {
+			if (vlan_tag_present) {
 				key_vlan->vlan_id = skb_vlan_tag_get_id(skb);
 				key_vlan->vlan_priority =
 					(skb_vlan_tag_get_prio(skb) >> VLAN_PRIO_SHIFT);

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH net-next] flow_dissector: fix vlan tag handling
  2016-10-22 20:30           ` [PATCH net-next] flow_dissector: fix vlan tag handling Arnd Bergmann
@ 2016-10-24  8:17             ` Jiri Pirko
  2016-10-24 16:00               ` Arnd Bergmann
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Pirko @ 2016-10-24  8:17 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Tom Herbert, David S. Miller, Alexander Duyck, Jiri Pirko,
	Hadar Hen Zion, Gao Feng, Amir Vadai,
	Linux Kernel Network Developers, LKML, Linus Torvalds

Sat, Oct 22, 2016 at 10:30:08PM CEST, arnd@arndb.de wrote:
>gcc warns about an uninitialized pointer dereference in the vlan
>priority handling:
>
>net/core/flow_dissector.c: In function '__skb_flow_dissect':
>net/core/flow_dissector.c:281:61: error: 'vlan' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
>As pointed out by Jiri Pirko, the variable is never actually used
>without being initialized first as the only way it end up uninitialized
>is with skb_vlan_tag_present(skb)==true, and that means it does not
>get accessed.
>
>However, the warning hints at some related issues that I'm addressing
>here:
>
>- the second check for the vlan tag is different from the first one
>  that tests the skb for being NULL first, causing both the warning
>  and a possible NULL pointer dereference that was not entirely fixed.
>- The same patch that introduced the NULL pointer check dropped an
>  earlier optimization that skipped the repeated check of the
>  protocol type
>- The local '_vlan' variable is referenced through the 'vlan' pointer
>  but the variable has gone out of scope by the time that it is
>  accessed, causing undefined behavior as the stack may have been
>  overwritten.
>
>Caching the result of the 'skb && skb_vlan_tag_present(skb)' check
>in a local variable allows the compiler to further optimize the
>later check. With those changes, the warning also disappears.
>
>Fixes: 3805a938a6c2 ("flow_dissector: Check skb for VLAN only if skb specified.")
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>---
>
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 44e6ba9d3a6b..17be1b66cc41 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -246,13 +246,10 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 	case htons(ETH_P_8021AD):
> 	case htons(ETH_P_8021Q): {
> 		const struct vlan_hdr *vlan;
>+		struct vlan_hdr _vlan;
>+		bool vlan_tag_present = (skb && skb_vlan_tag_present(skb));

Drop the unnecessary "()"


> 
>-		if (skb && skb_vlan_tag_present(skb))
>-			proto = skb->protocol;

This does not look correct. I believe that you need to set proto for
further processing.


>-
>-		if (eth_type_vlan(proto)) {
>-			struct vlan_hdr _vlan;
>-
>+		if (!vlan_tag_present || eth_type_vlan(skb->protocol)) {
> 			vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan),
> 						    data, hlen, &_vlan);
> 			if (!vlan)
>@@ -270,7 +267,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 							     FLOW_DISSECTOR_KEY_VLAN,
> 							     target_container);
> 
>-			if (skb_vlan_tag_present(skb)) {
>+			if (vlan_tag_present) {
> 				key_vlan->vlan_id = skb_vlan_tag_get_id(skb);
> 				key_vlan->vlan_priority =
> 					(skb_vlan_tag_get_prio(skb) >> VLAN_PRIO_SHIFT);
>

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH net-next] flow_dissector: fix vlan tag handling
  2016-10-24  8:17             ` Jiri Pirko
@ 2016-10-24 16:00               ` Arnd Bergmann
  0 siblings, 0 replies; 11+ messages in thread
From: Arnd Bergmann @ 2016-10-24 16:00 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Tom Herbert, David S. Miller, Alexander Duyck, Jiri Pirko,
	Hadar Hen Zion, Gao Feng, Amir Vadai,
	Linux Kernel Network Developers, LKML, Linus Torvalds

On Monday, October 24, 2016 10:17:36 AM CEST Jiri Pirko wrote:
> Sat, Oct 22, 2016 at 10:30:08PM CEST, arnd@arndb.de wrote:
> >diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> >index 44e6ba9d3a6b..17be1b66cc41 100644
> >--- a/net/core/flow_dissector.c
> >+++ b/net/core/flow_dissector.c
> >@@ -246,13 +246,10 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> > 	case htons(ETH_P_8021AD):
> > 	case htons(ETH_P_8021Q): {
> > 		const struct vlan_hdr *vlan;
> >+		struct vlan_hdr _vlan;
> >+		bool vlan_tag_present = (skb && skb_vlan_tag_present(skb));
> 
> Drop the unnecessary "()"

ok

> > 
> >-		if (skb && skb_vlan_tag_present(skb))
> >-			proto = skb->protocol;
> 
> This does not look correct. I believe that you need to set proto for
> further processing.
> 

Ah, of course. I only looked at the usage in this 'case' statement,
but the variable is also used after the 'goto again' and at
the end of the function.

	Arnd

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-10-24 16:06 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-21 15:55 [PATCH] flow_dissector: avoid uninitialized variable access Arnd Bergmann
2016-10-21 16:31 ` Jiri Pirko
2016-10-21 21:05   ` Arnd Bergmann
2016-10-21 22:16     ` Arnd Bergmann
2016-10-22 15:57       ` Eric Garver
2016-10-22 18:20         ` Tom Herbert
2016-10-22 20:30           ` [PATCH net-next] flow_dissector: fix vlan tag handling Arnd Bergmann
2016-10-24  8:17             ` Jiri Pirko
2016-10-24 16:00               ` Arnd Bergmann
2016-10-22  1:48   ` [PATCH] flow_dissector: avoid uninitialized variable access Linus Torvalds
2016-10-22  6:55     ` Jiri Pirko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).