Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1635869 > unrolled thread

[net-ipv4] question about arguments position

Back to article view | Back to linux.kernel

Contents

  [net-ipv4] question about arguments position "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-04 18:10 +0200
    Re: [net-ipv4] question about arguments position David Miller <davem@davemloft.net> - 2017-05-04 18:50 +0200
      Re: [net-ipv4] question about arguments position Joe Perches <joe@perches.com> - 2017-05-04 19:50 +0200
        Re: [net-ipv4] question about arguments position Joe Perches <joe@perches.com> - 2017-05-04 21:10 +0200
          Re: [net-ipv4] question about arguments position Joe Perches <joe@perches.com> - 2017-05-04 21:20 +0200
            Re: [net-ipv4] question about arguments position "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-04 21:30 +0200
              [PATCH] net: ipv4: add code comment for clarification "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-04 22:10 +0200
                Re: [PATCH] net: ipv4: add code comment for clarification David Miller <davem@davemloft.net> - 2017-05-08 17:40 +0200
                  Re: [PATCH] net: ipv4: add code comment for clarification "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-08 18:10 +0200
          Re: [net-ipv4] question about arguments position "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-04 21:20 +0200
        Re: [net-ipv4] question about arguments position "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-04 21:50 +0200
      Re: [net-ipv4] question about arguments position "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-04 20:00 +0200

#1635869 — [net-ipv4] question about arguments position

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

[toc] | [next] | [standalone]

#1635884

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.

[toc] | [prev] | [next] | [standalone]

#1635929

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.

[toc] | [prev] | [next] | [standalone]

#1635976

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.

[toc] | [prev] | [next] | [standalone]

#1635980

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.

[toc] | [prev] | [next] | [standalone]

#1635987

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

[toc] | [prev] | [next] | [standalone]

#1636011 — [PATCH] 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 <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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1635984

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

[toc] | [prev] | [next] | [standalone]

#1635992

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

[toc] | [prev] | [next] | [standalone]

#1635947

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web