Skip to contentSkip to conversation

question about arguments position

net-ipv4 In review Gustavo A. R. Silva 3 participants 12 messages May 4, 2017 – May 8, 2017

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

Hello everybody,

While looking into Coverity ID 1357474 I ran into the following piece of code at net/ipv4/inet_diag.c:392:

struct sock *inet_diag_find_one_icsk(struct net *net,
struct inet_hashinfo *hashinfo, const struct inet_diag_req_v2 *req)

{
        struct sock *sk;

        rcu_read_lock();
        if (req->sdiag_family == AF_INET)
                sk = inet_lookup(net, hashinfo, NULL, 0, req->id.idiag_dst[0],
                                 req->id.idiag_dport, req->id.idiag_src[0],
                                 req->id.idiag_sport, req->id.idiag_if);

#if IS_ENABLED(CONFIG_IPV6)
else if (req->sdiag_family == AF_INET6) {
if (ipv6_addr_v4mapped((struct in6_addr
*)req->id.idiag_dst) &&
ipv6_addr_v4mapped((struct in6_addr *)req->id.idiag_src))
sk = inet_lookup(net, hashinfo, NULL, 0,
req->id.idiag_dst[3],

                                         req->id.idiag_dport,  
req->id.idiag_src[3],
                                         req->id.idiag_sport,

req->id.idiag_if);
else
sk = inet6_lookup(net, hashinfo, NULL, 0,
(struct in6_addr
*)req->id.idiag_dst,

                                          req->id.idiag_dport,
                                          (struct in6_addr  
*)req->id.idiag_src,
                                          req->id.idiag_sport,
                                          req->id.idiag_if);
        }

#endif

The issue here is that the position of arguments in the call to inet_lookup() and inet6_lookup() functions do not match the order of the parameters:

req->id.idiag_dport is passed to sport
req->id.idiag_sport is passed to dport

These are the function prototypes:

static inline struct sock *inet_lookup(struct net *net,
struct inet_hashinfo *hashinfo, struct sk_buff *skb, int doff, const __be32 saddr, const __be16 sport, const __be32 daddr, const __be16 dport, const int dif)

struct sock *inet6_lookup(struct net *net, struct inet_hashinfo *hashinfo,
struct sk_buff *skb, int doff,
const struct in6_addr *saddr, const __be16 sport, const struct in6_addr *daddr, const __be16 dport, const int dif)

My question here is if this is intentional?

In case it is not, I will send a patch to fix it. But first it would be great to hear any comment about it.

Thank you!
--
Gustavo A. R. Silva

maintainer
#1 lore raw

From: "Gustavo A. R. Silva" <[email protected]>
Date: Thu, 04 May 2017 11:07:54 -0500

Quoted 2 lines

While looking into Coverity ID 1357474 I ran into the following piece of code at net/ipv4/inet_diag.c:392:

Because it's been this way since at least 2005, it doesn't matter if the order is correct or not. What's there is the locked in behavior exposed to userspace and changing it will break things for people.

author
#2 lore raw

Hi David,

Quoting David Miller <[email protected]>:

Quoted 9 lines

From: "Gustavo A. R. Silva" <[email protected]>
Date: Thu, 04 May 2017 11:07:54 -0500

Quoted 2 lines

While looking into Coverity ID 1357474 I ran into the following piece of code at net/ipv4/inet_diag.c:392:

Because it's been this way since at least 2005, it doesn't matter if the order is correct or not. What's there is the locked in behavior exposed to userspace and changing it will break things for people.

Oh, I see.

Thanks for clarifying

#3 lore raw
In reply to David Miller
Quoted 9 lines · David Miller

From: "Gustavo A. R. Silva" <[email protected]>
Date: Thu, 04 May 2017 11:07:54 -0500

Quoted 2 lines

While looking into Coverity ID 1357474 I ran into the following piece of code at net/ipv4/inet_diag.c:392:

Because it's been this way since at least 2005, it doesn't matter if the order is correct or not.  What's there is the locked in behavior exposed to userspace and changing it will break things for people.

Adding a few comments around the code about why
it is this way will help avoid future questions.

author
#4 lore raw

Hi Joe,

Quoting Joe Perches <[email protected]>:

Quoted 7 lines · David Miller
Quoted 6 lines

From: "Gustavo A. R. Silva" <[email protected]>
Date: Thu, 04 May 2017 11:07:54 -0500

Quoted 2 lines

While looking into Coverity ID 1357474 I ran into the following piece of code at net/ipv4/inet_diag.c:392:

Quoted 6 lines
Quoted 3 lines

Because it's been this way since at least 2005, it doesn't matter if the order is correct or not.  What's there is the locked in behavior exposed to userspace and changing it will break things for people.

Adding a few comments around the code about why
it is this way will help avoid future questions.

In the case of Coverity, I already triaged and documented this issue. So people can ignore it in the future.

Regarding the code comments, what about the following patch:

net/ipv4/inet_diag.c +6 -0
index 3828b3a..7a56641 100644
@@ -389,6 +389,12 @@ static int sk_diag_fill(struct sock *sk, struct sk_buff *skb,
                                  nlmsg_flags, unlh, net_admin);
 }
 
+/*
+ * Ignore the position of the arguments req->id.idiag_dport and
+ * req->id.idiag_sport in both calls to inet_lookup() and inet6_lookup()
+ * functions, once this is a locked in behavior exposed to user space.
+ * Changing this will break things for people.
+ */
 struct sock *inet_diag_find_one_icsk(struct net *net,
                                     struct inet_hashinfo *hashinfo,
                                     const struct inet_diag_req_v2 *req)

Thanks

#5 lore raw
Quoted 1 line · Gustavo A. R. Silva

Regarding the code comments, what about the following patch:

[]

net/ipv4/inet_diag.c

Quoted 1 line
net/ipv4/inet_diag.c +0 -0

[]

Quoted 13 lines
net/ipv4/inet_diag.c +6 -0
@@ -389,6 +389,12 @@ static int sk_diag_fill(struct sock *sk, struct  
 sk_buff *skb,
                                   nlmsg_flags, unlh, net_admin);
  }
 
+/*
+ * Ignore the position of the arguments req->id.idiag_dport and
+ * req->id.idiag_sport in both calls to inet_lookup() and inet6_lookup()
+ * functions, once this is a locked in behavior exposed to user space.
+ * Changing this will break things for people.
+ */
  struct sock *inet_diag_find_one_icsk(struct net *net,
                                      struct inet_hashinfo *hashinfo,
Quoted 2 lines

const struct inet_diag_req_v2 *req)

Seems sensible. Thanks.

author
#6 lore raw

Quoting Joe Perches <[email protected]>:

Quoted 10 lines · Gustavo A. R. Silva
Quoted 1 line

Regarding the code comments, what about the following patch:

[]

net/ipv4/inet_diag.c

Quoted 1 line
net/ipv4/inet_diag.c +0 -0

[]

Quoted 5 lines
net/ipv4/inet_diag.c +0 -0
@@ -389,6 +389,12 @@ static int sk_diag_fill(struct sock *sk, struct
 sk_buff *skb,
                                   nlmsg_flags, unlh, net_admin);
  }
Quoted 12 lines
Quoted 10 lines
net/ipv4/inet_diag.c +6 -0
+/*
+ * Ignore the position of the arguments req->id.idiag_dport and
+ * req->id.idiag_sport in both calls to inet_lookup() and inet6_lookup()
+ * functions, once this is a locked in behavior exposed to user space.
+ * Changing this will break things for people.
+ */
  struct sock *inet_diag_find_one_icsk(struct net *net,
                                      struct inet_hashinfo *hashinfo,

const struct inet_diag_req_v2 *req)

Seems sensible. Thanks.

Should I resend it in a full and proper format or it can taken from here?

Thanks
--
Gustavo A. R. Silva

#7 lore raw
Quoted 27 lines · Gustavo A. R. Silva

Quoting Joe Perches <[email protected]>:

Quoted 12 lines · Gustavo A. R. Silva
Quoted 1 line

Regarding the code comments, what about the following patch:

[]

net/ipv4/inet_diag.c

Quoted 1 line
net/ipv4/inet_diag.c +0 -0

[]

Quoted 5 lines
net/ipv4/inet_diag.c +0 -0
@@ -389,6 +389,12 @@ static int sk_diag_fill(struct sock *sk, struct
 sk_buff *skb,
                                   nlmsg_flags, unlh, net_admin);
  }
Quoted 12 lines
Quoted 10 lines
net/ipv4/inet_diag.c +6 -0
+/*
+ * Ignore the position of the arguments req->id.idiag_dport and
+ * req->id.idiag_sport in both calls to inet_lookup() and inet6_lookup()
+ * functions, once this is a locked in behavior exposed to user space.
+ * Changing this will break things for people.
+ */
  struct sock *inet_diag_find_one_icsk(struct net *net,
                                      struct inet_hashinfo *hashinfo,

const struct inet_diag_req_v2 *req)

Seems sensible. Thanks.

Quoted 1 line

Should I resend it in a full and proper format or it can taken from here?

If you want it applied, it should be resent as a full patch with your sign-off.

author
#8 lore raw

Quoting Joe Perches <[email protected]>:

[]

Quoted 13 lines
Quoted 13 lines
Quoted 3 lines
Quoted 3 lines

+/*
+ * Ignore the position of the arguments req->id.idiag_dport and
+ * req->id.idiag_sport in both calls to inet_lookup() and

inet6_lookup()

Quoted 6 lines
Quoted 6 lines

+ * functions, once this is a locked in behavior exposed to user space.
+ * Changing this will break things for people.
+ */
struct sock *inet_diag_find_one_icsk(struct net *net,
struct inet_hashinfo *hashinfo, const struct

inet_diag_req_v2 *req)

Quoted 2 lines
Quoted 1 line
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

Seems sensible. Thanks.

Should I resend it in a full and proper format or it can taken from here?

If you want it applied, it should be resent as a full patch with your sign-off.

I'll send it shortly.

Thanks for clarifying
--
Gustavo A. R. Silva

author
#9 lore raw

PATCH 1/1 net: ipv4: add code comment for clarification

Add code comment to make it clear that the position of the arguments req->id.idiag_dport and req->id.idiag_sport is a locked in behavior and it should not be changed.

Addresses-Coverity-ID: 1357474

Cc David Miller <[email protected]>
Cc Joe Perches <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 net/ipv4/inet_diag.c | 6 ++++++
 1 file changed, 6 insertions(+)
net/ipv4/inet_diag.c +6 -0
index 3828b3a..841800b 100644
@@ -389,6 +389,12 @@ static int sk_diag_fill(struct sock *sk, struct sk_buff *skb,
 				  nlmsg_flags, unlh, net_admin);
 }
 
+/*
+ * Ignore the position of the arguments req->id.idiag_dport and
+ * req->id.idiag_sport in both calls to inet_lookup() and inet6_lookup()
+ * functions, once this is a locked in behavior exposed to user space.
+ * Changing this will break things for people.
+ */
 struct sock *inet_diag_find_one_icsk(struct net *net,
 				     struct inet_hashinfo *hashinfo,
 				     const struct inet_diag_req_v2 *req)
Signature · 1 line
2.5.0
maintainer
#10 lore raw

Re: [PATCH] net: ipv4: add code comment for clarification

From: "Gustavo A. R. Silva" <[email protected]>
Date: Thu, 4 May 2017 14:44:16 -0500

net/ipv4/inet_diag.c static int sk_diag_fill(struct sock *sk, struct sk_buff *skb, · line 397

Quoted 10 lines
net/ipv4/inet_diag.c +6 -0
@@ -389,6 +389,12 @@ static int sk_diag_fill(struct sock *sk, struct sk_buff *skb,
 				  nlmsg_flags, unlh, net_admin);
 }
 
+/*
+ * Ignore the position of the arguments req->id.idiag_dport and
+ * req->id.idiag_sport in both calls to inet_lookup() and inet6_lookup()
+ * functions, once this is a locked in behavior exposed to user space.
+ * Changing this will break things for people.
+ */

This is implicit for every interface exposed to userspace.

Therefore, saying it here and there in various comments provides questionable value.

And in fact I think these arguments are probably in the correct order.

I'm definitely not applying a patch like this, sorry.

author
#11 lore raw

Re: [PATCH] net: ipv4: add code comment for clarification

Hi David,

Quoting David Miller <[email protected]>:

Quoted 17 lines

From: "Gustavo A. R. Silva" <[email protected]>
Date: Thu, 4 May 2017 14:44:16 -0500

net/ipv4/inet_diag.c static int sk_diag_fill(struct sock *sk, · line 398

Quoted 11 lines
net/ipv4/inet_diag.c +6 -0
@@ -389,6 +389,12 @@ static int sk_diag_fill(struct sock *sk,  
 struct sk_buff *skb,
 				  nlmsg_flags, unlh, net_admin);
 }
 
+/*
+ * Ignore the position of the arguments req->id.idiag_dport and
+ * req->id.idiag_sport in both calls to inet_lookup() and inet6_lookup()
+ * functions, once this is a locked in behavior exposed to user space.
+ * Changing this will break things for people.
+ */

This is implicit for every interface exposed to userspace.

Quoted 6 lines

Therefore, saying it here and there in various comments provides questionable value.

And in fact I think these arguments are probably in the correct order.

I'm definitely not applying a patch like this, sorry.

I get it, thanks for clarifying.