Skip to contentSkip to conversation

netfilter: ip_tables: remove useless variable assignment in get_info()

PATCH In review Gustavo A. R. Silva 2 participants 3 messages Jun 26, 2017 – Jun 27, 2017

Subsystems (3)
Networking Network drivers netfilter
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 netfilter: ip_tables: remove useless variable assignment in get_info()

Value assigned to variable _ret_ at line 970 is overwritten either at line 986 or 988, before it can be used. This makes such variable assignment useless.

Addresses-Coverity-ID: 1226932

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 net/ipv4/netfilter/ip_tables.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/ipv4/netfilter/ip_tables.c +1 -1
index 2a55a40..648697c 100644
@@ -967,7 +967,7 @@ static int get_info(struct net *net, void __user *user,
 		struct xt_table_info tmp;
 
 		if (compat) {
-			ret = compat_table_info(private, &tmp);
+			compat_table_info(private, &tmp);
 			xt_compat_flush_offsets(AF_INET);
 			private = &tmp;
 		}
Signature · 1 line
2.5.0
#1 lore raw
Quoted 5 lines · Gustavo A. R. Silva

Value assigned to variable _ret_ at line 970 is overwritten either at line 986 or 988, before it can be used. This makes such variable assignment useless.

Addresses-Coverity-ID: 1226932

[]

net/ipv4/netfilter/ip_tables.c

Quoted 1 line
net/ipv4/netfilter/ip_tables.c +0 -0

[]

net/ipv4/netfilter/ip_tables.c static int get_info(struct net *net, void __user *user, · line 970

Quoted 6 lines
net/ipv4/netfilter/ip_tables.c +1 -1
@@ -967,7 +967,7 @@ static int get_info(struct net *net, void __user *user,
 		struct xt_table_info tmp;
 
 		if (compat) {
-			ret = compat_table_info(private, &tmp);
+			compat_table_info(private, &tmp);

why isn't it more appropriate to test the return value?

Quoted 3 lines
net/ipv4/netfilter/ip_tables.c +0 -0
 			xt_compat_flush_offsets(AF_INET);
 			private = &tmp;
 		}
author
#2 lore raw

Hi Joe,

Quoting Joe Perches <[email protected]>:

Quoted 9 lines · Gustavo A. R. Silva
Quoted 5 lines

Value assigned to variable _ret_ at line 970 is overwritten either at line 986 or 988, before it can be used. This makes such variable assignment useless.

Addresses-Coverity-ID: 1226932

[]

net/ipv4/netfilter/ip_tables.c

Quoted 1 line
net/ipv4/netfilter/ip_tables.c +0 -0

[]

Quoted 9 lines

net/ipv4/netfilter/ip_tables.c static int get_info(struct net *net, void __user *user, · line 970

Quoted 6 lines
net/ipv4/netfilter/ip_tables.c +1 -1
@@ -967,7 +967,7 @@ static int get_info(struct net *net, void __user *user,
 		struct xt_table_info tmp;
 
 		if (compat) {
-			ret = compat_table_info(private, &tmp);
+			compat_table_info(private, &tmp);

why isn't it more appropriate to test the return value?

Oh, in this particular case, based on git blame, the code has been like that for more than 10 years. So my reasoning was that if it hasn't been fixed yet, maybe that return value is not relevant.

But in case it turns out to actually be relevant, what do you think about the following patch:

net/ipv4/netfilter/ip_tables.c +11 -4
@@ -968,7 +968,8 @@ static int get_info(struct net *net, void __user *user,
 
                if (compat) {
                        ret = compat_table_info(private, &tmp);
-                       xt_compat_flush_offsets(AF_INET);
+                       if (!ret)
+                               goto out;
                        private = &tmp;
                }
 #endif

@@ -986,14 +987,20 @@ static int get_info(struct net *net, void __user *user,
                        ret = -EFAULT;
                else
                        ret = 0;
+       } else
+               ret = -ENOENT;
 
+out:
+       if (t) {
                xt_table_unlock(t);
                module_put(t->me);
-       } else
-               ret = -ENOENT;
+       }
+
 #ifdef CONFIG_COMPAT
-       if (compat)
+       if (compat) {
+               xt_compat_flush_offsets(AF_INET);
                xt_compat_unlock(AF_INET);
+       }
 #endif
        return ret;
 }

Thank you!
--
Gustavo A. R. Silva