| Message ID | 496C445E.1000707@cn.fujitsu.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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,
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,
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
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,
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, >>> >> >> >> > > >
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.
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
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,
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,
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,
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. > >
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,
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
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
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,
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.^_^
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))) {
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(-)