[net-ipv4] question about arguments position

[net-ipv4] question about arguments position

[Gustavo A. R. Silva]

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

Re: [net-ipv4] question about arguments position

[David Miller]

From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date: Thu, 04 May 2017 11:07:54 -0500

> 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.

Re: [net-ipv4] question about arguments position

[Gustavo A. R. Silva]

Hi David,

Quoting David Miller <davem@davemloft.net>:

> From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
> Date: Thu, 04 May 2017 11:07:54 -0500
>
>> 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
--
Gustavo A. R. Silva

Re: [net-ipv4] question about arguments position

[Gustavo A. R. Silva]

Hi David,

Quoting David Miller <davem@davemloft.net>:

> From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
> Date: Thu, 04 May 2017 11:07:54 -0500
>
>> 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

Re: [net-ipv4] question about arguments position

[Joe Perches]

On Thu, 2017-05-04 at 12:46 -0400, David Miller wrote:
> From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
> Date: Thu, 04 May 2017 11:07:54 -0500
> 
> > 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.

Re: [net-ipv4] question about arguments position

[Gustavo A. R. Silva]

Hi Joe,

Quoting Joe Perches <joe@perches.com>:

> On Thu, 2017-05-04 at 12:46 -0400, David Miller wrote:
>> From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
>> Date: Thu, 04 May 2017 11:07:54 -0500
>>
>> > 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.

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:

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 3828b3a..7a56641 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -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
--
Gustavo A. R. Silva

Re: [net-ipv4] question about arguments position

[Gustavo A. R. Silva]

Hi Joe,

Quoting Joe Perches <joe@perches.com>:

> On Thu, 2017-05-04 at 12:46 -0400, David Miller wrote:
>> From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
>> Date: Thu, 04 May 2017 11:07:54 -0500
>>
>> > 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.

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:

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 3828b3a..7a56641 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -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

Re: [net-ipv4] question about arguments position

[Joe Perches]

On Thu, 2017-05-04 at 14:00 -0500, Gustavo A. R. Silva wrote:
> Regarding the code comments, what about the following patch:
[]
> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
[]
> @@ -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)
> 

Seems sensible.  Thanks.

Re: [net-ipv4] question about arguments position

[Gustavo A. R. Silva]

Quoting Joe Perches <joe@perches.com>:

> On Thu, 2017-05-04 at 14:00 -0500, Gustavo A. R. Silva wrote:
>> Regarding the code comments, what about the following patch:
> []
>> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
> []
>> @@ -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)
>>
>
> Seems sensible.  Thanks.

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

Thanks
--
Gustavo A. R. Silva

Re: [net-ipv4] question about arguments position

[Joe Perches]

On Thu, 2017-05-04 at 14:15 -0500, Gustavo A. R. Silva wrote:
> Quoting Joe Perches <joe@perches.com>:
> 
> > On Thu, 2017-05-04 at 14:00 -0500, Gustavo A. R. Silva wrote:
> > > Regarding the code comments, what about the following patch:
> > 
> > []
> > > diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
> > 
> > []
> > > @@ -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)
> > > 
> > 
> > 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.

Re: [net-ipv4] question about arguments position

[Gustavo A. R. Silva]

Quoting Joe Perches <joe@perches.com>:

[]
>> > > +/*
>> > > + * 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?
>
> 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

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

[Gustavo A. R. Silva]

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 <davem@davemloft.net>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 net/ipv4/inet_diag.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 3828b3a..841800b 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -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)
-- 
2.5.0

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

[David Miller]

From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date: Thu, 4 May 2017 14:44:16 -0500

> @@ -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.

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

[Gustavo A. R. Silva]

Hi David,

Quoting David Miller <davem@davemloft.net>:

> From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
> Date: Thu, 4 May 2017 14:44:16 -0500
>
>> @@ -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.

I get it, thanks for clarifying.
--
Gustavo A. R. Silva

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

[Gustavo A. R. Silva]

Hi David,

Quoting David Miller <davem@davemloft.net>:

> From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
> Date: Thu, 4 May 2017 14:44:16 -0500
>
>> @@ -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.

I get it, thanks for clarifying.