| Message ID | 1323393883-3759-1-git-send-email-xi.wang@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 12/08/2011 08:24 PM, Xi Wang wrote: > The commit 8ffd3208 voids the previous patches f6778aab and 810c0719 > for limiting the maximum autoclose value. If userspace passes in -1 > on 32-bit platform, the overflow check didn't work and autoclose > would be set to 0xffffffff. > > Signed-off-by: Xi Wang <xi.wang@gmail.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Andrei Pelinescu-Onciul <andrei@iptel.org> > Cc: Vlad Yasevich <vladislav.yasevich@hp.com> > Cc: David S. Miller <davem@davemloft.net> > --- > net/sctp/socket.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index 13bf5fc..bb91281 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -2201,7 +2201,8 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval, > if (copy_from_user(&sp->autoclose, optval, optlen)) > return -EFAULT; > /* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */ > - sp->autoclose = min_t(long, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ); > + sp->autoclose = min_t(unsigned long, sp->autoclose, > + MAX_SCHEDULE_TIMEOUT / HZ); I think this should be u32 since that's what sp->autoclose is. -vlad > > return 0; > } -- 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 Dec 9, 2011, at 12:38 PM, Vladislav Yasevich wrote:
> I think this should be u32 since that's what sp->autoclose is.
Do you mean something like this?
min_t(u32, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ)
On 64-bit platform this would limit autoclose for no good
reason to
(u32)(MAX_SCHEDULE_TIMEOUT / HZ),
which is basically 0x978d4fdf (assuming HZ is 250). I guess the
intention was to allow autoclose to be any u32 on 64-bit platform.
- xi
--
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 12/09/2011 01:04 PM, Xi Wang wrote: > On Dec 9, 2011, at 12:38 PM, Vladislav Yasevich wrote: >> I think this should be u32 since that's what sp->autoclose is. > > Do you mean something like this? > > min_t(u32, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ) > > On 64-bit platform this would limit autoclose for no good > reason to > > (u32)(MAX_SCHEDULE_TIMEOUT / HZ), > > which is basically 0x978d4fdf (assuming HZ is 250). I guess the > intention was to allow autoclose to be any u32 on 64-bit platform. > Hm.. this is a bit strange. This makes it so that on 32 bit platforms we have one upper bound for autoclose and on 64 we have another even though the type is platform dependent. This could be considered a regression by applications. In addition this would result in confusion to user since the values between setsockopt() and getsockopt() for autoclose would be different. Looking at the latest spec, it actually looks like we are completely mis-interpreting autoclose. It's supposed to be in seconds. For now, I'd suggest to make this consistent between 32 and 64 bits. Having inconsistent values seems strange. As a next set the api needs to be fixed to accept seconds as argument. -vlad -- 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 Dec 12, 2011, at 5:18 PM, Vladislav Yasevich wrote: > Hm.. this is a bit strange. This makes it so that on 32 bit platforms > we have one upper bound for autoclose and on 64 we have another even though > the type is platform dependent. This could be considered a regression by > applications. Either looks good to me. Timeout limit is essentially different on 32/64 platforms. Another (probably uglier) option is to limit the value on 32-bit platform only, like sock_setsockopt() in net/core/sock.c. #if (BITS_PER_LONG == 32) if (sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ) sp->autoclose = MAX_SCHEDULE_TIMEOUT / HZ; #endif > In addition this would result in confusion to user since the values > between setsockopt() and getsockopt() for autoclose would be different. Are you suggesting to reject the value and return -EINVAL, rather than silently limiting the autoclose value? - xi -- 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 12/13/2011 05:00 PM, Xi Wang wrote: > On Dec 12, 2011, at 5:18 PM, Vladislav Yasevich wrote: >> Hm.. this is a bit strange. This makes it so that on 32 bit platforms >> we have one upper bound for autoclose and on 64 we have another even though >> the type is platform dependent. This could be considered a regression by >> applications. > > Either looks good to me. Timeout limit is essentially different on 32/64 > platforms. I don't think it really should be different. Notice that our rto values remain consistent. I really thing that this should be consistent from the user's point of view. > > Another (probably uglier) option is to limit the value on 32-bit platform > only, like sock_setsockopt() in net/core/sock.c. > > #if (BITS_PER_LONG == 32) > if (sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ) > sp->autoclose = MAX_SCHEDULE_TIMEOUT / HZ; > #endif I agree, this is ugly. It might make more sense to define a max autoclose value and expose it through /sys. That way the values remains consistent. -vlad > >> In addition this would result in confusion to user since the values >> between setsockopt() and getsockopt() for autoclose would be different. > > Are you suggesting to reject the value and return -EINVAL, rather than > silently limiting the autoclose value? > > - xi > -- 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 Dec 13, 2011, at 5:15 PM, Vladislav Yasevich wrote: > I agree, this is ugly. It might make more sense to define a max autoclose > value and expose it through /sys. That way the values remains consistent. Hmm, this sounds nice. I will give it a try in v2. - xi -- 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
diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 13bf5fc..bb91281 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -2201,7 +2201,8 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval, if (copy_from_user(&sp->autoclose, optval, optlen)) return -EFAULT; /* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */ - sp->autoclose = min_t(long, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ); + sp->autoclose = min_t(unsigned long, sp->autoclose, + MAX_SCHEDULE_TIMEOUT / HZ); return 0; }
The commit 8ffd3208 voids the previous patches f6778aab and 810c0719 for limiting the maximum autoclose value. If userspace passes in -1 on 32-bit platform, the overflow check didn't work and autoclose would be set to 0xffffffff. Signed-off-by: Xi Wang <xi.wang@gmail.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Andrei Pelinescu-Onciul <andrei@iptel.org> Cc: Vlad Yasevich <vladislav.yasevich@hp.com> Cc: David S. Miller <davem@davemloft.net> --- net/sctp/socket.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)