packet: bail out of packet_snd() if L2 header creation fails
Subsystems (4)
PATCH 1/1 packet: bail out of packet_snd() if L2 header creation fails
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").
--- net/packet/af_packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/packet/af_packet.c +1 -1
@@ -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))
Signature · 1 line
2.1.0
Quoted 12 lines · 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.
Quoted 5 lines
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 <[email protected]>---
Nice catch !
Willem de Bruijn
Acked-byQuoted 15 lines · Eric Dumazet
Quoted 14 lines · 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").
Quoted 8 lines
Quoted 3 lines
Fixes 9c7077622dd9 ("packet: make packet_snd fail on len smaller than l2 header")Signed-off-by Christoph Jaeger <[email protected]>---
Nice catch !
Acked-by Eric Dumazet <[email protected]>
Indeed. I'm responsible for that typo. Thanks a lot for catching it!
Joe Perches
maintainerQuoted 7 lines · Christoph Jaeger
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)
Quoted 5 lines · Joe Perches
Quoted 4 lines · Christoph Jaeger
Due to a misplaced parenthesis, the expression
(unlikely(offset) < 0),
Quoted 7 lines
Quoted 3 lines
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
Joe Perches
maintainerPATCH 1/1 checkpatch: Add likely/unlikely comparison misuse test
Add a test for probably likely/unlikely misuses where the comparison is likely misplaced
if (likely(foo) > 0) vs if (likely(foo > 0))
---
Quoted 7 lines · Christoph Jaeger
Quoted 1 line
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(+)
scripts/checkpatch.pl +7 -0
@@ -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/@) {
Quoted 8 lines · Christoph Jaeger
Due to a misplaced parenthesis, the expression
(unlikely(offset) < 0),which expands to
(__builtin_expect(!!(offset), 0) < 0),
Quoted 8 lines
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 <[email protected]>
Thanks, Christoph!
From: Christoph Jaeger <[email protected]>
Date: Sun, 11 Jan 2015 13:01:16 -0500
Quoted 8 lines
Due to a misplaced parenthesis, the expression
(unlikely(offset) < 0),
which expands to
(
__builtin_expect(!!(offset), 0)< 0),
Quoted 8 lines
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 <[email protected]>
Applied, thank you.