[PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Yang Hongyang]

Actually the condition (optlen == 0) is included in (optlen < sizeof(struct in6_pktinfo)),
so we do not need to check it separately.

Signed-off-by: Yang Hongyang<yanghy@cn.fujitsu.com>

---
 net/ipv6/ipv6_sockglue.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 40f3246..e6affa7 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -399,9 +399,7 @@ sticky_done:
 	{
 		struct in6_pktinfo pkt;
 
-		if (optlen == 0)
-			goto e_inval;
-		else if (optlen < sizeof(struct in6_pktinfo) || optval == NULL)
+		if (optlen < sizeof(struct in6_pktinfo) || optval == NULL)
 			goto e_inval;
 
 		if (copy_from_user(&pkt, optval, sizeof(struct in6_pktinfo))) {
-- 
1.6.0.3
-- 
Regards
Yang Hongyang

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Herbert Xu]

Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
> Actually the condition (optlen == 0) is included in (optlen < sizeof(struct in6_pktinfo)),
> so we do not need to check it separately.

You don't need to check optval == NULL either since that's the
job of copy_from_user.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Yang Hongyang]

Herbert Xu wrote:
> Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
>> Actually the condition (optlen == 0) is included in (optlen < sizeof(struct in6_pktinfo)),
>> so we do not need to check it separately.
> 
> You don't need to check optval == NULL either since that's the
> job of copy_from_user.

If optval==NULL, what we should return?EINVAL or EFAULT?
If it is EINVAL,then we should check it .otherwise it's the job of 
copy_from_user

> 
> Cheers,


-- 
Regards
Yang Hongyang

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Wei Yongjun]

Yang Hongyang wrote:
> Herbert Xu wrote:
>   
>> Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
>>     
>>> Actually the condition (optlen == 0) is included in (optlen < sizeof(struct in6_pktinfo)),
>>> so we do not need to check it separately.
>>>       
>> You don't need to check optval == NULL either since that's the
>> job of copy_from_user.
>>     
>
> If optval==NULL, what we should return?EINVAL or EFAULT?
> If it is EINVAL,then we should check it .otherwise it's the job of 
> copy_from_user
>   

I think if optval==NULL, the in6_pktinfo which is set should be remove. 
So, you should handle optval==NULL. Not just return error.

>   
>> Cheers,
>>     
>
>
>   

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Yang Hongyang]

Wei Yongjun wrote:
> Yang Hongyang wrote:
>> Herbert Xu wrote:
>>  
>>> Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
>>>    
>>>> Actually the condition (optlen == 0) is included in (optlen <
>>>> sizeof(struct in6_pktinfo)),
>>>> so we do not need to check it separately.
>>>>       
>>> You don't need to check optval == NULL either since that's the
>>> job of copy_from_user.
>>>     
>>
>> If optval==NULL, what we should return?EINVAL or EFAULT?
>> If it is EINVAL,then we should check it .otherwise it's the job of
>> copy_from_user
>>   
> 
> I think if optval==NULL, the in6_pktinfo which is set should be remove.
> So, you should handle optval==NULL. Not just return error.

There's no RFC defines the behavior above,but:
  RFC3542 said The application can remove any sticky Routing header or sticky 
Destination options header or sticky Hop-by-Hop options header by calling 
setsockopt() with a zero option length.

So,do we need to allow remove any sticky pktinfo option by calling 
setsockopt() with a zero option length?

> 
>>  
>>> Cheers,
>>>     
>>
>>
>>   
> 
> 
> 


-- 
Regards
Yang Hongyang

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Shan Wei]

Yang Hongyang said:
> Wei Yongjun wrote:
>> Yang Hongyang wrote:
>>> Herbert Xu wrote:
>>>  
>>>> Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
>>>>    
>>>>> Actually the condition (optlen == 0) is included in (optlen <
>>>>> sizeof(struct in6_pktinfo)),
>>>>> so we do not need to check it separately.
>>>>>       
>>>> You don't need to check optval == NULL either since that's the
>>>> job of copy_from_user.
>>>>     
>>> If optval==NULL, what we should return?EINVAL or EFAULT?
>>> If it is EINVAL,then we should check it .otherwise it's the job of
>>> copy_from_user
>>>   
>> I think if optval==NULL, the in6_pktinfo which is set should be remove.
>> So, you should handle optval==NULL. Not just return error.
> 
> There's no RFC defines the behavior above,but:
>   RFC3542 said The application can remove any sticky Routing header or sticky 
> Destination options header or sticky Hop-by-Hop options header by calling 
> setsockopt() with a zero option length.
> 
> So,do we need to allow remove any sticky pktinfo option by calling 
> setsockopt() with a zero option length?
> 

Can remove the option using seting in6_pktinfo struct wiht 
ipi6_ifindex=0,ipi6_ifindex=IN6ADDR_ANY_INIT.

If no RFC definition, not to reset with optlen=0,
for example IPV6_TCLASS, PV6_2292DSTOPTS.

-- 
Best Regards
Shan Wei

Re: [PATCHv2]IPv6:remove duplicate check when setsockopt with IPV6_PKTINFO option

[Yang Hongyang]

Actually the condition (optlen == 0) is included in (optlen < sizeof(struct in6_pktinfo)),
so we do not need to check it separately.
We don't need to check (optval==NULL) either,because copy_from_user() will do the job.

Signed-off-by: Yang Hongyang<yanghy@cn.fujitsu.com>

---
 net/ipv6/ipv6_sockglue.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 40f3246..1c203b6 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -399,9 +399,7 @@ sticky_done:
 	{
 		struct in6_pktinfo pkt;
 
-		if (optlen == 0)
-			goto e_inval;
-		else if (optlen < sizeof(struct in6_pktinfo) || optval == NULL)
+		if (optlen < sizeof(struct in6_pktinfo))
 			goto e_inval;
 
 		if (copy_from_user(&pkt, optval, sizeof(struct in6_pktinfo))) {
-- 
1.6.0.3

Shan Wei wrote:
> Yang Hongyang said:
>> Wei Yongjun wrote:
>>> Yang Hongyang wrote:
>>>> Herbert Xu wrote:
>>>>  
>>>>> Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
>>>>>    
>>>>>> Actually the condition (optlen == 0) is included in (optlen <
>>>>>> sizeof(struct in6_pktinfo)),
>>>>>> so we do not need to check it separately.
>>>>>>       
>>>>> You don't need to check optval == NULL either since that's the
>>>>> job of copy_from_user.
>>>>>     
>>>> If optval==NULL, what we should return?EINVAL or EFAULT?
>>>> If it is EINVAL,then we should check it .otherwise it's the job of
>>>> copy_from_user
>>>>   
>>> I think if optval==NULL, the in6_pktinfo which is set should be remove.
>>> So, you should handle optval==NULL. Not just return error.
>> There's no RFC defines the behavior above,but:
>>   RFC3542 said The application can remove any sticky Routing header or sticky 
>> Destination options header or sticky Hop-by-Hop options header by calling 
>> setsockopt() with a zero option length.
>>
>> So,do we need to allow remove any sticky pktinfo option by calling 
>> setsockopt() with a zero option length?
>>
> 
> Can remove the option using seting in6_pktinfo struct wiht 
> ipi6_ifindex=0,ipi6_ifindex=IN6ADDR_ANY_INIT.
> 
> If no RFC definition, not to reset with optlen=0,
> for example IPV6_TCLASS, PV6_2292DSTOPTS.
> 


-- 
Regards
Yang Hongyang

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Herbert Xu]

Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
>
> If optval==NULL, what we should return?EINVAL or EFAULT?
> If it is EINVAL,then we should check it .otherwise it's the job of 
> copy_from_user

I think EFAULT is fine.  We return that elsewhere as well.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 14 Jan 2009 15:48:56 +1100

> Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
> >
> > If optval==NULL, what we should return?EINVAL or EFAULT?
> > If it is EINVAL,then we should check it .otherwise it's the job of 
> > copy_from_user
> 
> I think EFAULT is fine.  We return that elsewhere as well.

Actually, we return EINVAL just a few lines above this code
block for some other socket option cases when optval==NULL.

So for consistency I'm applying Yang's original patch to
net-next-2.6

Thanks.

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Herbert Xu]

On Wed, Jan 14, 2009 at 09:02:42PM -0800, David Miller wrote:
>
> Actually, we return EINVAL just a few lines above this code
> block for some other socket option cases when optval==NULL.

Well the very next option IPV6_2292PKTOPTIONS returns EFAULT
by virtue of not explicitly checking optval == NULL :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Herbert Xu]

On Thu, Jan 15, 2009 at 04:04:49PM +1100, Herbert Xu wrote:
> On Wed, Jan 14, 2009 at 09:02:42PM -0800, David Miller wrote:
> >
> > Actually, we return EINVAL just a few lines above this code
> > block for some other socket option cases when optval==NULL.
> 
> Well the very next option IPV6_2292PKTOPTIONS returns EFAULT
> by virtue of not explicitly checking optval == NULL :)

In fact checking for a NULL pointer is strictly speaking wrong
since NULL may actually have been mapped in user-space :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Yang Hongyang]

Herbert Xu wrote:
> On Thu, Jan 15, 2009 at 04:04:49PM +1100, Herbert Xu wrote:
>> On Wed, Jan 14, 2009 at 09:02:42PM -0800, David Miller wrote:
>>> Actually, we return EINVAL just a few lines above this code
>>> block for some other socket option cases when optval==NULL.
>> Well the very next option IPV6_2292PKTOPTIONS returns EFAULT
>> by virtue of not explicitly checking optval == NULL :)
> 
> In fact checking for a NULL pointer is strictly speaking wrong
> since NULL may actually have been mapped in user-space :)

But there are some cases that user-space passes a NULL pointer
to the kernel,Otherwise,copy_from_user needn't to cheak the
NULL pointer either.:)

By the way,I think most part of the ipv6 socket option implementation
are kind of ugly:)

> 
> Cheers,


-- 
Regards
Yang Hongyang

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 15 Jan 2009 16:04:49 +1100

> On Wed, Jan 14, 2009 at 09:02:42PM -0800, David Miller wrote:
> >
> > Actually, we return EINVAL just a few lines above this code
> > block for some other socket option cases when optval==NULL.
> 
> Well the very next option IPV6_2292PKTOPTIONS returns EFAULT
> by virtue of not explicitly checking optval == NULL :)

Touche' :-)

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Yang Hongyang]

David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Wed, 14 Jan 2009 15:48:56 +1100
> 
>> Yang Hongyang <yanghy@cn.fujitsu.com> wrote:
>>> If optval==NULL, what we should return?EINVAL or EFAULT?
>>> If it is EINVAL,then we should check it .otherwise it's the job of 
>>> copy_from_user
>> I think EFAULT is fine.  We return that elsewhere as well.
> 
> Actually, we return EINVAL just a few lines above this code
> block for some other socket option cases when optval==NULL.
> 
> So for consistency I'm applying Yang's original patch to
> net-next-2.6

Dave,there are some of the code elsewhere return EFAULT when
optval=NULL,I will post a patch to fix them:)

> 
> Thanks.
> 
> 


-- 
Regards
Yang Hongyang

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Herbert Xu]

On Thu, Jan 15, 2009 at 01:34:03PM +0800, Yang Hongyang wrote:
> 
> Dave,there are some of the code elsewhere return EFAULT when
> optval=NULL,I will post a patch to fix them:)

I disagree.  The concept of a user-space NULL pointer is different
to that of a kernel-space NULL pointer, so the kernel should not
be treating a user-space NULL pointer differently than any other
pointer.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 15 Jan 2009 16:37:02 +1100

> On Thu, Jan 15, 2009 at 01:34:03PM +0800, Yang Hongyang wrote:
> > 
> > Dave,there are some of the code elsewhere return EFAULT when
> > optval=NULL,I will post a patch to fix them:)
> 
> I disagree.  The concept of a user-space NULL pointer is different
> to that of a kernel-space NULL pointer, so the kernel should not
> be treating a user-space NULL pointer differently than any other
> pointer.

I agree with Herbert, but of course we have to adhere to cases
where APIs actually define a user NULL pointer to have a specific
meaning.

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Herbert Xu]

On Wed, Jan 14, 2009 at 09:45:21PM -0800, David Miller wrote:
>
> I agree with Herbert, but of course we have to adhere to cases
> where APIs actually define a user NULL pointer to have a specific
> meaning.

Absolutely.  Although AFAIK the relevant RFCs here only use the
notion of a zero option length for such purposes rather than
NULL pointers (presumably because the meaning of NULL pointers
are context dependent).

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH]IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

[Yang Hongyang]

David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Thu, 15 Jan 2009 16:37:02 +1100
> 
>> On Thu, Jan 15, 2009 at 01:34:03PM +0800, Yang Hongyang wrote:
>>> Dave,there are some of the code elsewhere return EFAULT when
>>> optval=NULL,I will post a patch to fix them:)
>> I disagree.  The concept of a user-space NULL pointer is different
>> to that of a kernel-space NULL pointer, so the kernel should not
>> be treating a user-space NULL pointer differently than any other
>> pointer.
> 
> I agree with Herbert, but of course we have to adhere to cases
> where APIs actually define a user NULL pointer to have a specific
> meaning.
> 
> 

Agreed.Seems that Patch v2 is correct according to the discussion.^_^

-- 
Regards
Yang Hongyang