On Mon, 23 Nov 2009 16:06:50 -0500 Vlad Yasevich <vladislav.yasevich@xxxxxx> wrote: > Andrei Pelinescu-Onciul (3): > sctp: allow setting path_maxrxt independent of SPP_PMTUD_ENABLE > sctp: limit maximum autoclose setsockopt value > sctp: fix integer overflow when setting the autoclose timer Problems with this one: : commit f6778aab6ccc4b510b4dcfa770d9949b696b4545 : Author: Andrei Pelinescu-Onciul <andrei@xxxxxxxxx> : AuthorDate: Mon Nov 23 15:54:01 2009 -0500 : Commit: Vlad Yasevich <vladislav.yasevich@xxxxxx> : CommitDate: Mon Nov 23 15:54:01 2009 -0500 : : sctp: limit maximum autoclose setsockopt value : : To avoid overflowing the maximum timer interval when transforming : the autoclose interval from seconds to jiffies, limit the maximum : autoclose value to MAX_SCHEDULE_TIMEOUT/HZ. : : Signed-off-by: Andrei Pelinescu-Onciul <andrei@xxxxxxxxx> : Signed-off-by: Vlad Yasevich <vladislav.yasevich@xxxxxx> : : diff --git a/net/sctp/socket.c b/net/sctp/socket.c : index d2681a6..71513b3 100644 : --- a/net/sctp/socket.c : +++ b/net/sctp/socket.c : @@ -2086,6 +2086,9 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval, : return -EINVAL; : if (copy_from_user(&sp->autoclose, optval, optlen)) : return -EFAULT; : + /* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */ : + if (sp->autoclose > (MAX_SCHEDULE_TIMEOUT / HZ) ) : + sp->autoclose = MAX_SCHEDULE_TIMEOUT / HZ ; : : return 0; : } a) it has two coding-style errors in two lines. Please go away, add scripts/checkpatch.pl to your patch development tools and then continue reading. b) have you done that yet? c) it generates this on 64-bit: net/sctp/socket.c: In function 'sctp_setsockopt_autoclose': net/sctp/socket.c:2090: warning: comparison is always false due to limited range of data type but that's proving somewhat hard to fix in a nice way. d) I'm not sure that we should fix it anyway. Is it really a good idea to take an incorrect, invalid setting from userspace, to silently modify that setting and to not inform userspace? Bear in mind that MAX_SCHEDULE_TIMEOUT has different values on 32- and 64-bit kernels. So the same source code will have different behaviour depending on what type of kernel it is executed on. I think. It also means that kernel behaviour will differ as CONFIG_HZ is altered, in some way which I can't be bothered working out. Overall, it would be way simpler and saner to clamp this value to some explicit time period, IMO. <pulls number out of thin air> --- a/net/sctp/socket.c~a +++ a/net/sctp/socket.c @@ -2086,9 +2086,8 @@ static int sctp_setsockopt_autoclose(str return -EINVAL; if (copy_from_user(&sp->autoclose, optval, optlen)) return -EFAULT; - /* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */ - if (sp->autoclose > (MAX_SCHEDULE_TIMEOUT / HZ) ) - sp->autoclose = MAX_SCHEDULE_TIMEOUT / HZ ; + /* make sure it won't exceed one hour */ + sp->autoclose = min_t(u32, sp->autoclose, 60 * 60); return 0; } _ -- To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
