[PATCH net] packet: bail out of packet_snd() if L2 header creation fails

[PATCH net] packet: bail out of packet_snd() if L2 header creation fails

[Christoph Jaeger]

Due to a misplaced parenthesis, the expression

  (unlikely(offset) < 0),

which expands to

  (__builtin_expect(!!(offset), 0) < 0),

never evaluates to true. Therefore, when sending packets with
PF_PACKET/SOCK_DGRAM, packet_snd() does not abort as intended
if the creation of the layer 2 header fails.

Spotted by Coverity - CID 1259975 ("Operands don't affect result").

Fixes: 9c7077622dd9 ("packet: make packet_snd fail on len smaller than l2 header")
Signed-off-by: Christoph Jaeger <cj@linux.com>
---
 net/packet/af_packet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 6880f34..9cfe2e1 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2517,7 +2517,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 	err = -EINVAL;
 	if (sock->type == SOCK_DGRAM) {
 		offset = dev_hard_header(skb, dev, ntohs(proto), addr, NULL, len);
-		if (unlikely(offset) < 0)
+		if (unlikely(offset < 0))
 			goto out_free;
 	} else {
 		if (ll_header_truncated(dev, len))
-- 
2.1.0

Re: [PATCH net] packet: bail out of packet_snd() if L2 header creation fails

[Eric Dumazet]

On Sun, 2015-01-11 at 13:01 -0500, Christoph Jaeger wrote:
> Due to a misplaced parenthesis, the expression
> 
>   (unlikely(offset) < 0),
> 
> which expands to
> 
>   (__builtin_expect(!!(offset), 0) < 0),
> 
> never evaluates to true. Therefore, when sending packets with
> PF_PACKET/SOCK_DGRAM, packet_snd() does not abort as intended
> if the creation of the layer 2 header fails.
> 
> Spotted by Coverity - CID 1259975 ("Operands don't affect result").
> 
> Fixes: 9c7077622dd9 ("packet: make packet_snd fail on len smaller than l2 header")
> Signed-off-by: Christoph Jaeger <cj@linux.com>
> ---

Nice catch !

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net] packet: bail out of packet_snd() if L2 header creation fails

[Willem de Bruijn]

On Sun, Jan 11, 2015 at 1:35 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2015-01-11 at 13:01 -0500, Christoph Jaeger wrote:
>> Due to a misplaced parenthesis, the expression
>>
>>   (unlikely(offset) < 0),
>>
>> which expands to
>>
>>   (__builtin_expect(!!(offset), 0) < 0),
>>
>> never evaluates to true. Therefore, when sending packets with
>> PF_PACKET/SOCK_DGRAM, packet_snd() does not abort as intended
>> if the creation of the layer 2 header fails.
>>
>> Spotted by Coverity - CID 1259975 ("Operands don't affect result").
>>
>> Fixes: 9c7077622dd9 ("packet: make packet_snd fail on len smaller than l2 header")
>> Signed-off-by: Christoph Jaeger <cj@linux.com>
>> ---
>
> Nice catch !
>
> Acked-by: Eric Dumazet <edumazet@google.com>
>

Indeed. I'm responsible for that typo. Thanks a lot for catching it!

Acked-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net] packet: bail out of packet_snd() if L2 header creation fails

[Joe Perches]

On Sun, 2015-01-11 at 13:01 -0500, Christoph Jaeger wrote:
> Due to a misplaced parenthesis, the expression
> 
>   (unlikely(offset) < 0),
> 
> which expands to
> 
>   (__builtin_expect(!!(offset), 0) < 0),

Here's another one:

drivers/platform/goldfish/goldfish_pipe.c:285:	if (unlikely(bufflen) == 0)

Re: [PATCH net] packet: bail out of packet_snd() if L2 header creation fails

[Christoph Jaeger]

On Sun, Jan 11, 2015 at 10:52:25AM -0800, Joe Perches wrote:
> On Sun, 2015-01-11 at 13:01 -0500, Christoph Jaeger wrote:
> > Due to a misplaced parenthesis, the expression
> > 
> >   (unlikely(offset) < 0),
> > 
> > which expands to
> > 
> >   (__builtin_expect(!!(offset), 0) < 0),
> 
> Here's another one:
> 
> drivers/platform/goldfish/goldfish_pipe.c:285:	if (unlikely(bufflen) == 0)

Well, the conditional statement works as intended. Of course, the branch
prediction doesn't.

Coccinelle should be able to check for this kind of likely()/unlikely() usage,
shouldn't it?

~Christoph

[PATCH] checkpatch: Add likely/unlikely comparison misuse test

[Joe Perches]

Add a test for probably likely/unlikely misuses where
the comparison is likely misplaced

	if (likely(foo) > 0)
vs
	if (likely(foo > 0))

Signed-off-by: Joe Perches <joe@perches.com>
---
On Sun, 2015-01-11 at 14:34 -0500, Christoph Jaeger wrote:
> > drivers/platform/goldfish/goldfish_pipe.c:285:	if (unlikely(bufflen) == 0)
> 
> Well, the conditional statement works as intended. Of course, the branch
> prediction doesn't.
> 
> Coccinelle should be able to check for this kind of likely()/unlikely() usage,
> shouldn't it?

Most likely,  checkpatch could too, but not as well.
This misuse isn't very common. (2 in current source?)

 scripts/checkpatch.pl | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6afc24b..b8d47dc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5219,6 +5219,13 @@ sub process {
 			      "#define of '$1' is wrong - use Kconfig variables or standard guards instead\n" . $herecurr);
 		}
 
+# likely/unlikely comparisons similar to "(likely(foo) > 0)"
+		if ($^V && $^V ge 5.10.0 &&
+		    $line =~ /\b((?:un)?likely)\s*\(\s*$FuncArg\s*\)\s*$Compare/) {
+			WARN("LIKELY_MISUSE",
+			     "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
+		}
+
 # whine mightly about in_atomic
 		if ($line =~ /\bin_atomic\s*\(/) {
 			if ($realfile =~ m@^drivers/@) {

Re: [PATCH net] packet: bail out of packet_snd() if L2 header creation fails

[Daniel Borkmann]

On 01/11/2015 07:01 PM, Christoph Jaeger wrote:
> Due to a misplaced parenthesis, the expression
>
>    (unlikely(offset) < 0),
>
> which expands to
>
>    (__builtin_expect(!!(offset), 0) < 0),
>
> never evaluates to true. Therefore, when sending packets with
> PF_PACKET/SOCK_DGRAM, packet_snd() does not abort as intended
> if the creation of the layer 2 header fails.
>
> Spotted by Coverity - CID 1259975 ("Operands don't affect result").
>
> Fixes: 9c7077622dd9 ("packet: make packet_snd fail on len smaller than l2 header")
> Signed-off-by: Christoph Jaeger <cj@linux.com>

Thanks, Christoph!

Acked-by: Daniel Borkmann <dborkman@redhat.com>

Re: [PATCH net] packet: bail out of packet_snd() if L2 header creation fails

[David Miller]

From: Christoph Jaeger <cj@linux.com>
Date: Sun, 11 Jan 2015 13:01:16 -0500

> Due to a misplaced parenthesis, the expression
> 
>   (unlikely(offset) < 0),
> 
> which expands to
> 
>   (__builtin_expect(!!(offset), 0) < 0),
> 
> never evaluates to true. Therefore, when sending packets with
> PF_PACKET/SOCK_DGRAM, packet_snd() does not abort as intended
> if the creation of the layer 2 header fails.
> 
> Spotted by Coverity - CID 1259975 ("Operands don't affect result").
> 
> Fixes: 9c7077622dd9 ("packet: make packet_snd fail on len smaller than l2 header")
> Signed-off-by: Christoph Jaeger <cj@linux.com>

Applied, thank you.