* [PATCH] ipv4: avoid undefined behavior in do_ip_setsockopt()
@ 2012-11-11 21:20 Xi Wang
2012-11-11 22:02 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Xi Wang @ 2012-11-11 21:20 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Xi Wang
(1<<optname) is undefined behavior in C with a negative optname or
optname larger than 31. In those cases the result of the shift is
not necessarily zero (e.g., on x86).
This patch simplifies the code with a switch statement on optname.
It also allows the compiler to generate better code (e.g., using a
64-bit mask).
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
net/ipv4/ip_sockglue.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index 5eea4a8..14bbfcf 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -457,19 +457,28 @@ static int do_ip_setsockopt(struct sock *sk, int level,
struct inet_sock *inet = inet_sk(sk);
int val = 0, err;
- if (((1<<optname) & ((1<<IP_PKTINFO) | (1<<IP_RECVTTL) |
- (1<<IP_RECVOPTS) | (1<<IP_RECVTOS) |
- (1<<IP_RETOPTS) | (1<<IP_TOS) |
- (1<<IP_TTL) | (1<<IP_HDRINCL) |
- (1<<IP_MTU_DISCOVER) | (1<<IP_RECVERR) |
- (1<<IP_ROUTER_ALERT) | (1<<IP_FREEBIND) |
- (1<<IP_PASSSEC) | (1<<IP_TRANSPARENT) |
- (1<<IP_MINTTL) | (1<<IP_NODEFRAG))) ||
- optname == IP_UNICAST_IF ||
- optname == IP_MULTICAST_TTL ||
- optname == IP_MULTICAST_ALL ||
- optname == IP_MULTICAST_LOOP ||
- optname == IP_RECVORIGDSTADDR) {
+ switch (optname) {
+ case IP_PKTINFO:
+ case IP_RECVTTL:
+ case IP_RECVOPTS:
+ case IP_RECVTOS:
+ case IP_RETOPTS:
+ case IP_TOS:
+ case IP_TTL:
+ case IP_HDRINCL:
+ case IP_MTU_DISCOVER:
+ case IP_RECVERR:
+ case IP_ROUTER_ALERT:
+ case IP_FREEBIND:
+ case IP_PASSSEC:
+ case IP_TRANSPARENT:
+ case IP_MINTTL:
+ case IP_NODEFRAG:
+ case IP_UNICAST_IF:
+ case IP_MULTICAST_TTL:
+ case IP_MULTICAST_ALL:
+ case IP_MULTICAST_LOOP:
+ case IP_RECVORIGDSTADDR:
if (optlen >= sizeof(int)) {
if (get_user(val, (int __user *) optval))
return -EFAULT;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ipv4: avoid undefined behavior in do_ip_setsockopt()
2012-11-11 21:20 [PATCH] ipv4: avoid undefined behavior in do_ip_setsockopt() Xi Wang
@ 2012-11-11 22:02 ` David Miller
2012-11-11 22:18 ` Xi Wang
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2012-11-11 22:02 UTC (permalink / raw)
To: xi.wang; +Cc: netdev
This code is a fast bit test on purpose. You're making the
code slower.
I'm not applying this patch.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipv4: avoid undefined behavior in do_ip_setsockopt()
2012-11-11 22:02 ` David Miller
@ 2012-11-11 22:18 ` Xi Wang
2012-11-11 22:50 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Xi Wang @ 2012-11-11 22:18 UTC (permalink / raw)
To: David Miller; +Cc: netdev
On 11/11/12 5:02 PM, David Miller wrote:
>
> This code is a fast bit test on purpose. You're making the
> code slower.
No, it's the opposite. All modern compilers optimize switch cases
into a fast bit test. The original "smarter" code, however, hinders
the compiler determining the mask for a fast bit test. With this
patch, GCC is able to compute a better mask. You can check the
assembly.
- xi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipv4: avoid undefined behavior in do_ip_setsockopt()
2012-11-11 22:18 ` Xi Wang
@ 2012-11-11 22:50 ` David Miller
2012-11-12 13:54 ` David Laight
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2012-11-11 22:50 UTC (permalink / raw)
To: xi.wang; +Cc: netdev
From: Xi Wang <xi.wang@gmail.com>
Date: Sun, 11 Nov 2012 17:18:55 -0500
> On 11/11/12 5:02 PM, David Miller wrote:
>>
>> This code is a fast bit test on purpose. You're making the
>> code slower.
>
> No, it's the opposite. All modern compilers optimize switch cases
> into a fast bit test.
Indeed, I even checked sparc64 with gcc-4.6 and it looks good.
Thanks for the clarification, I'll apply this, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] ipv4: avoid undefined behavior in do_ip_setsockopt()
2012-11-11 22:50 ` David Miller
@ 2012-11-12 13:54 ` David Laight
2012-11-12 17:00 ` Xi Wang
0 siblings, 1 reply; 6+ messages in thread
From: David Laight @ 2012-11-12 13:54 UTC (permalink / raw)
To: David Miller, xi.wang; +Cc: netdev
> >> This code is a fast bit test on purpose. You're making the
> >> code slower.
> >
> > No, it's the opposite. All modern compilers optimize switch cases
> > into a fast bit test.
'All modern' is probably an overstatement, 'recent gcc' might be valid.
> Indeed, I even checked sparc64 with gcc-4.6 and it looks good.
>
> Thanks for the clarification, I'll apply this, thanks.
The 'switch' version will have an extra conditional to detect
'out of range' values - even though we know they can't happen.
I'm not sure you can avoid that - even for an enum.
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipv4: avoid undefined behavior in do_ip_setsockopt()
2012-11-12 13:54 ` David Laight
@ 2012-11-12 17:00 ` Xi Wang
0 siblings, 0 replies; 6+ messages in thread
From: Xi Wang @ 2012-11-12 17:00 UTC (permalink / raw)
To: David Laight; +Cc: David Miller, netdev
On 11/12/12 8:54 AM, David Laight wrote:
> 'All modern' is probably an overstatement, 'recent gcc' might be valid.
I agree if you consider gcc 3.4 released 8 years ago as "recent gcc",
or if you use a compiler other than gcc/clang/icc to compile the kernel.
> The 'switch' version will have an extra conditional to detect
> 'out of range' values - even though we know they can't happen.
> I'm not sure you can avoid that - even for an enum.
This out-of-range check is exactly what this patch wanted to add:
optname is a syscall parameter, and we should reject invalid optname
values before doing (1<<optname).
- xi
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-11-12 17:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-11 21:20 [PATCH] ipv4: avoid undefined behavior in do_ip_setsockopt() Xi Wang
2012-11-11 22:02 ` David Miller
2012-11-11 22:18 ` Xi Wang
2012-11-11 22:50 ` David Miller
2012-11-12 13:54 ` David Laight
2012-11-12 17:00 ` Xi Wang
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).