Patchwork IPv6:remove duplicate check of optlen when setsockopt with IPV6_PKTINFO option

login
register
mail settings
Submitter Yang Hongyang
Date Jan. 13, 2009, 7:35 a.m.
Message ID <496C445E.1000707@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/18180/
State Accepted
Delegated to: David Miller
Headers show

Comments

Yang Hongyang - Jan. 13, 2009, 7:35 a.m.
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(-)
Herbert Xu - Jan. 14, 2009, 3:47 a.m.
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,
Yang Hongyang - Jan. 14, 2009, 3:54 a.m.
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,
Wei Yongjun - Jan. 14, 2009, 4:07 a.m.
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,
>>     
>
>
>   

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu - Jan. 14, 2009, 4:48 a.m.
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,
Yang Hongyang - Jan. 14, 2009, 5:27 a.m.
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,
>>>     
>>
>>
>>   
> 
> 
>
Shan Wei - Jan. 14, 2009, 8:26 a.m.
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.
David Miller - Jan. 15, 2009, 5:02 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu - Jan. 15, 2009, 5:04 a.m.
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,
Herbert Xu - Jan. 15, 2009, 5:06 a.m.
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,
Yang Hongyang - Jan. 15, 2009, 5:25 a.m.
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,
Yang Hongyang - Jan. 15, 2009, 5:34 a.m.
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.
> 
>
Herbert Xu - Jan. 15, 2009, 5:37 a.m.
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,
David Miller - Jan. 15, 2009, 5:43 a.m.
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' :-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 15, 2009, 5:45 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu - Jan. 15, 2009, 5:48 a.m.
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,
Yang Hongyang - Jan. 15, 2009, 10:07 a.m.
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.^_^

Patch

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))) {