* [Patch v7 0/3] net: reserve ports for applications using fixed port numbers @ 2010-04-09 10:10 Amerigo Wang 2010-04-09 10:11 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Amerigo Wang @ 2010-04-09 10:10 UTC (permalink / raw) To: linux-kernel Cc: Octavian Purdila, ebiederm, Eric Dumazet, netdev, Neil Horman, Amerigo Wang, David Miller Changes from the previous version: - Update to latest Linus' tree; - Address Eric B.'s concern, copy the whole bitmap instead of set one by one. -------------> This patch introduces /proc/sys/net/ipv4/ip_local_reserved_ports which allows users to reserve ports for third-party applications. The reserved ports will not be used by automatic port assignments (e.g. when calling connect() or bind() with port number 0). Explicit port allocation behavior is unchanged. There are still some miss behaviors with regard to proc parsing in odd invalid cases (for "40000\0-40001" all is acknowledged but only 40000 is accepted) but they are not easy to fix without changing the current "acknowledge how much we accepted" behavior. Because of that and because the same issues are present in the existing proc_dointvec code as well I don't think its worth holding the actual feature (port reservation) after such petty error recovery issues. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Patch 1/3] sysctl: refactor integer handling proc code 2010-04-09 10:10 [Patch v7 0/3] net: reserve ports for applications using fixed port numbers Amerigo Wang @ 2010-04-09 10:11 ` Amerigo Wang 2010-04-09 10:49 ` Changli Gao 2010-04-09 10:11 ` [Patch 2/3] sysctl: add proc_do_large_bitmap Amerigo Wang 2010-04-09 10:11 ` [Patch 3/3] net: reserve ports for applications using fixed port numbers Amerigo Wang 2 siblings, 1 reply; 24+ messages in thread From: Amerigo Wang @ 2010-04-09 10:11 UTC (permalink / raw) To: linux-kernel Cc: Octavian Purdila, Eric Dumazet, netdev, Neil Horman, Amerigo Wang, David Miller, ebiederm From: Octavian Purdila <opurdila@ixiacom.com> As we are about to add another integer handling proc function a little bit of cleanup is in order: add a few helper functions to improve code readability and decrease code duplication. In the process a bug is also fixed: if the user specifies a number with more then 20 digits it will be interpreted as two integers (e.g. 10000...13 will be interpreted as 100.... and 13). Behavior for EFAULT handling was changed as well. Previous to this patch, when an EFAULT error occurred in the middle of a write operation, although some of the elements were set, that was not acknowledged to the user (by shorting the write and returning the number of bytes accepted). EFAULT is now treated just like any other errors by acknowledging the amount of bytes accepted. Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> Signed-off-by: WANG Cong <amwang@redhat.com> Cc: Eric W. Biederman <ebiederm@xmission.com> --- Index: linux-2.6/kernel/sysctl.c =================================================================== --- linux-2.6.orig/kernel/sysctl.c +++ linux-2.6/kernel/sysctl.c @@ -2040,8 +2040,148 @@ int proc_dostring(struct ctl_table *tabl buffer, lenp, ppos); } +static int proc_skip_wspace(char __user **buf, size_t *size) +{ + char c; + + while (*size) { + if (get_user(c, *buf)) + return -EFAULT; + if (!isspace(c)) + break; + (*size)--; + (*buf)++; + } + + return 0; +} + +static bool isanyof(char c, const char *v, unsigned len) +{ + int i; + + if (!len) + return false; + + for (i = 0; i < len; i++) + if (c == v[i]) + break; + if (i == len) + return false; + + return true; +} + +#define TMPBUFLEN 22 +/** + * proc_get_ulong - reads an ASCII formated integer from a user buffer + * + * @buf - user buffer + * @size - size of the user buffer + * @val - this is where the number will be stored + * @neg - set to %TRUE if number is negative + * @perm_tr - a vector which contains the allowed trailers + * @perm_tr_len - size of the perm_tr vector + * @tr - pointer to store the trailer character + * + * In case of success 0 is returned and buf and size are updated with + * the amount of bytes read. If tr is non NULL and a trailing + * character exist (size is non zero after returning from this + * function) tr is updated with the trailing character. + */ +static int proc_get_ulong(char __user **buf, size_t *size, + unsigned long *val, bool *neg, + const char *perm_tr, unsigned perm_tr_len, char *tr) +{ + int len; + char *p, tmp[TMPBUFLEN]; + + if (!*size) + return -EINVAL; + + len = *size; + if (len > TMPBUFLEN-1) + len = TMPBUFLEN-1; + + if (copy_from_user(tmp, *buf, len)) + return -EFAULT; + + tmp[len] = 0; + p = tmp; + if (*p == '-' && *size > 1) { + *neg = 1; + p++; + } else + *neg = 0; + if (!isdigit(*p)) + return -EINVAL; + + *val = simple_strtoul(p, &p, 0); + + len = p - tmp; + + /* We don't know if the next char is whitespace thus we may accept + * invalid integers (e.g. 1234...a) or two integers instead of one + * (e.g. 123...1). So lets not allow such large numbers. */ + if (len == TMPBUFLEN - 1) + return -EINVAL; + + if (len < *size && perm_tr_len && !isanyof(*p, perm_tr, perm_tr_len)) + return -EINVAL; + + if (tr && (len < *size)) + *tr = *p; + + *buf += len; + *size -= len; + + return 0; +} + +/** + * proc_put_ulong - coverts an integer to a decimal ASCII formated string + * + * @buf - the user buffer + * @size - the size of the user buffer + * @val - the integer to be converted + * @neg - sign of the number, %TRUE for negative + * @first - if %FALSE will insert a separator character before the number + * @separator - the separator character + * + * In case of success 0 is returned and buf and size are updated with + * the amount of bytes read. + */ +static int proc_put_ulong(char __user **buf, size_t *size, unsigned long val, + bool neg, bool first, char separator) +{ + int len; + char tmp[TMPBUFLEN], *p = tmp; + + if (!first) + *p++ = separator; + sprintf(p, "%s%lu", neg ? "-" : "", val); + len = strlen(tmp); + if (len > *size) + len = *size; + if (copy_to_user(*buf, tmp, len)) + return -EFAULT; + *size -= len; + *buf += len; + return 0; +} +#undef TMPBUFLEN + +static int proc_put_char(char __user **buf, size_t *size, char c) +{ + if (*size) { + if (put_user(c, *buf)) + return -EFAULT; + (*size)--, (*buf)++; + } + return 0; +} -static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2050,7 +2190,7 @@ static int do_proc_dointvec_conv(int *ne } else { int val = *valp; if (val < 0) { - *negp = -1; + *negp = 1; *lvalp = (unsigned long)-val; } else { *negp = 0; @@ -2060,20 +2200,18 @@ static int do_proc_dointvec_conv(int *ne return 0; } +static const char proc_wspace_sep[] = { ' ', '\t', '\n', 0 }; + static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table, - int write, void __user *buffer, + int write, void __user *_buffer, size_t *lenp, loff_t *ppos, - int (*conv)(int *negp, unsigned long *lvalp, int *valp, + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, int write, void *data), void *data) { -#define TMPBUFLEN 21 - int *i, vleft, first = 1, neg; - unsigned long lval; - size_t left, len; - - char buf[TMPBUFLEN], *p; - char __user *s = buffer; + int *i, vleft, first = 1, err = 0; + size_t left; + char __user *buffer = (char __user *) _buffer; if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) { @@ -2089,88 +2227,48 @@ static int __do_proc_dointvec(void *tbl_ conv = do_proc_dointvec_conv; for (; left && vleft--; i++, first=0) { - if (write) { - while (left) { - char c; - if (get_user(c, s)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - s++; - } - if (!left) - break; - neg = 0; - len = left; - if (len > sizeof(buf) - 1) - len = sizeof(buf) - 1; - if (copy_from_user(buf, s, len)) - return -EFAULT; - buf[len] = 0; - p = buf; - if (*p == '-' && left > 1) { - neg = 1; - p++; - } - if (*p < '0' || *p > '9') - break; - - lval = simple_strtoul(p, &p, 0); + unsigned long lval; + bool neg; - len = p-buf; - if ((len < left) && *p && !isspace(*p)) + if (write) { + err = proc_skip_wspace(&buffer, &left); + if (err) + return err; + err = proc_get_ulong(&buffer, &left, &lval, &neg, + proc_wspace_sep, + sizeof(proc_wspace_sep), NULL); + if (err) break; - s += len; - left -= len; - - if (conv(&neg, &lval, i, 1, data)) + if (conv(&neg, &lval, i, 1, data)) { + err = -EINVAL; break; + } } else { - p = buf; - if (!first) - *p++ = '\t'; - - if (conv(&neg, &lval, i, 0, data)) + if (conv(&neg, &lval, i, 0, data)) { + err = -EINVAL; break; - - sprintf(p, "%s%lu", neg ? "-" : "", lval); - len = strlen(buf); - if (len > left) - len = left; - if(copy_to_user(s, buf, len)) - return -EFAULT; - left -= len; - s += len; - } - } - - if (!write && !first && left) { - if(put_user('\n', s)) - return -EFAULT; - left--, s++; - } - if (write) { - while (left) { - char c; - if (get_user(c, s++)) - return -EFAULT; - if (!isspace(c)) + } + err = proc_put_ulong(&buffer, &left, lval, neg, first, + '\t'); + if (err) break; - left--; } } + + if (!write && !first && left && !err) + err = proc_put_char(&buffer, &left, '\n'); + if (write && !err) + err = proc_skip_wspace(&buffer, &left); if (write && first) - return -EINVAL; + return err ? : -EINVAL; *lenp -= left; *ppos += *lenp; return 0; -#undef TMPBUFLEN } static int do_proc_dointvec(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos, - int (*conv)(int *negp, unsigned long *lvalp, int *valp, + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, int write, void *data), void *data) { @@ -2238,8 +2336,8 @@ struct do_proc_dointvec_minmax_conv_para int *max; }; -static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp, - int *valp, +static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp, + int *valp, int write, void *data) { struct do_proc_dointvec_minmax_conv_param *param = data; @@ -2252,7 +2350,7 @@ static int do_proc_dointvec_minmax_conv( } else { int val = *valp; if (val < 0) { - *negp = -1; + *negp = 1; *lvalp = (unsigned long)-val; } else { *negp = 0; @@ -2290,17 +2388,15 @@ int proc_dointvec_minmax(struct ctl_tabl } static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write, - void __user *buffer, + void __user *_buffer, size_t *lenp, loff_t *ppos, unsigned long convmul, unsigned long convdiv) { -#define TMPBUFLEN 21 - unsigned long *i, *min, *max, val; - int vleft, first=1, neg; - size_t len, left; - char buf[TMPBUFLEN], *p; - char __user *s = buffer; + unsigned long *i, *min, *max; + int vleft, first = 1, err = 0; + size_t left; + char __user *buffer = (char __user *) _buffer; if (!data || !table->maxlen || !*lenp || (*ppos && !write)) { @@ -2315,82 +2411,42 @@ static int __do_proc_doulongvec_minmax(v left = *lenp; for (; left && vleft--; i++, min++, max++, first=0) { + unsigned long val; + if (write) { - while (left) { - char c; - if (get_user(c, s)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - s++; - } - if (!left) - break; - neg = 0; - len = left; - if (len > TMPBUFLEN-1) - len = TMPBUFLEN-1; - if (copy_from_user(buf, s, len)) - return -EFAULT; - buf[len] = 0; - p = buf; - if (*p == '-' && left > 1) { - neg = 1; - p++; - } - if (*p < '0' || *p > '9') - break; - val = simple_strtoul(p, &p, 0) * convmul / convdiv ; - len = p-buf; - if ((len < left) && *p && !isspace(*p)) + bool neg; + + err = proc_skip_wspace(&buffer, &left); + if (err) + return err; + err = proc_get_ulong(&buffer, &left, &val, &neg, + proc_wspace_sep, + sizeof(proc_wspace_sep), NULL); + if (err) break; if (neg) - val = -val; - s += len; - left -= len; - - if(neg) continue; if ((min && val < *min) || (max && val > *max)) continue; *i = val; } else { - p = buf; - if (!first) - *p++ = '\t'; - sprintf(p, "%lu", convdiv * (*i) / convmul); - len = strlen(buf); - if (len > left) - len = left; - if(copy_to_user(s, buf, len)) - return -EFAULT; - left -= len; - s += len; - } - } - - if (!write && !first && left) { - if(put_user('\n', s)) - return -EFAULT; - left--, s++; - } - if (write) { - while (left) { - char c; - if (get_user(c, s++)) - return -EFAULT; - if (!isspace(c)) + val = convdiv * (*i) / convmul; + err = proc_put_ulong(&buffer, &left, val, 0, first, + '\t'); + if (err) break; - left--; } } + + if (!write && !first && left && !err) + err = proc_put_char(&buffer, &left, '\n'); + if (write && !err) + err = proc_skip_wspace(&buffer, &left); if (write && first) - return -EINVAL; + return err ? : -EINVAL; *lenp -= left; *ppos += *lenp; return 0; -#undef TMPBUFLEN } static int do_proc_doulongvec_minmax(struct ctl_table *table, int write, @@ -2451,7 +2507,7 @@ int proc_doulongvec_ms_jiffies_minmax(st } -static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2463,7 +2519,7 @@ static int do_proc_dointvec_jiffies_conv int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0; @@ -2474,7 +2530,7 @@ static int do_proc_dointvec_jiffies_conv return 0; } -static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_userhz_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2486,7 +2542,7 @@ static int do_proc_dointvec_userhz_jiffi int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0; @@ -2497,7 +2553,7 @@ static int do_proc_dointvec_userhz_jiffi return 0; } -static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_ms_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2507,7 +2563,7 @@ static int do_proc_dointvec_ms_jiffies_c int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0; ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 1/3] sysctl: refactor integer handling proc code 2010-04-09 10:11 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang @ 2010-04-09 10:49 ` Changli Gao 0 siblings, 0 replies; 24+ messages in thread From: Changli Gao @ 2010-04-09 10:49 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, Octavian Purdila, Eric Dumazet, netdev, Neil Horman, David Miller, ebiederm [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 23278 bytes --] On Fri, Apr 9, 2010 at 6:11 PM, Amerigo Wang <amwang@redhat.com> wrote:>> From: Octavian Purdila <opurdila@ixiacom.com>>> As we are about to add another integer handling proc function a little> bit of cleanup is in order: add a few helper functions to improve code> readability and decrease code duplication.>> In the process a bug is also fixed: if the user specifies a number> with more then 20 digits it will be interpreted as two integers> (e.g. 10000...13 will be interpreted as 100.... and 13).>> Behavior for EFAULT handling was changed as well. Previous to this> patch, when an EFAULT error occurred in the middle of a write> operation, although some of the elements were set, that was not> acknowledged to the user (by shorting the write and returning the> number of bytes accepted). EFAULT is now treated just like any other> errors by acknowledging the amount of bytes accepted.>> Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>> Signed-off-by: WANG Cong <amwang@redhat.com>> Cc: Eric W. Biederman <ebiederm@xmission.com>> --->> Index: linux-2.6/kernel/sysctl.c> ===================================================================> --- linux-2.6.orig/kernel/sysctl.c> +++ linux-2.6/kernel/sysctl.c> @@ -2040,8 +2040,148 @@ int proc_dostring(struct ctl_table *tabl>                buffer, lenp, ppos);>  }>> +static int proc_skip_wspace(char __user **buf, size_t *size)> +{> +    char c;> +> +    while (*size) {> +        if (get_user(c, *buf))> +            return -EFAULT;> +        if (!isspace(c))> +            break;> +        (*size)--;> +        (*buf)++;> +    }> +> +    return 0;> +}> +> +static bool isanyof(char c, const char *v, unsigned len)> +{> +    int i;> +> +    if (!len)> +        return false;> +> +    for (i = 0; i < len; i++)> +        if (c == v[i])> +            break;> +    if (i == len)> +        return false;> +> +    return true;> +}> +> +#define TMPBUFLEN 22> +/**> + * proc_get_ulong - reads an ASCII formated integer from a user buffer> + *> + * @buf - user buffer> + * @size - size of the user buffer> + * @val - this is where the number will be stored> + * @neg - set to %TRUE if number is negative> + * @perm_tr - a vector which contains the allowed trailers> + * @perm_tr_len - size of the perm_tr vector> + * @tr - pointer to store the trailer character> + *> + * In case of success 0 is returned and buf and size are updated with> + * the amount of bytes read. If tr is non NULL and a trailing> + * character exist (size is non zero after returning from this> + * function) tr is updated with the trailing character.> + */> +static int proc_get_ulong(char __user **buf, size_t *size,> +             unsigned long *val, bool *neg,> +             const char *perm_tr, unsigned perm_tr_len, char *tr)> +{> +    int len;> +    char *p, tmp[TMPBUFLEN];> +> +    if (!*size)> +        return -EINVAL;> +> +    len = *size;> +    if (len > TMPBUFLEN-1)> +        len = TMPBUFLEN-1;> +> +    if (copy_from_user(tmp, *buf, len))> +        return -EFAULT;> +> +    tmp[len] = 0;> +    p = tmp;> +    if (*p == '-' && *size > 1) {> +        *neg = 1;> +        p++;> +    } else> +        *neg = 0; the function name implies that it is used to parse unsigned long, sonegative value should not be supported. > +    if (!isdigit(*p))> +        return -EINVAL; It seems that ledding white space should be allowed, so this checkisn't needed, and simple_strtoul can handle it. > +> +    *val = simple_strtoul(p, &p, 0);> +> +    len = p - tmp;> +> +    /* We don't know if the next char is whitespace thus we may accept> +     * invalid integers (e.g. 1234...a) or two integers instead of one> +     * (e.g. 123...1). So lets not allow such large numbers. */> +    if (len == TMPBUFLEN - 1)> +        return -EINVAL;> +> +    if (len < *size && perm_tr_len && !isanyof(*p, perm_tr, perm_tr_len))> +        return -EINVAL; is strspn() better? > +> +    if (tr && (len < *size))> +        *tr = *p;> +> +    *buf += len;> +    *size -= len;> +> +    return 0;> +}> +> +/**> + * proc_put_ulong - coverts an integer to a decimal ASCII formated string> + *> + * @buf - the user buffer> + * @size - the size of the user buffer> + * @val - the integer to be converted> + * @neg - sign of the number, %TRUE for negative> + * @first - if %FALSE will insert a separator character before the number> + * @separator - the separator character> + *> + * In case of success 0 is returned and buf and size are updated with> + * the amount of bytes read.> + */> +static int proc_put_ulong(char __user **buf, size_t *size, unsigned long val,> +             bool neg, bool first, char separator)> +{> +    int len;> +    char tmp[TMPBUFLEN], *p = tmp;> +> +    if (!first)> +        *p++ = separator;> +    sprintf(p, "%s%lu", neg ? "-" : "", val); negative should not be supported too. > +    len = strlen(tmp);> +    if (len > *size)> +        len = *size;> +    if (copy_to_user(*buf, tmp, len))> +        return -EFAULT;> +    *size -= len;> +    *buf += len;> +    return 0;> +}> +#undef TMPBUFLEN> +> +static int proc_put_char(char __user **buf, size_t *size, char c)> +{> +    if (*size) {> +        if (put_user(c, *buf))> +            return -EFAULT;> +        (*size)--, (*buf)++;> +    }> +    return 0;> +}>> -static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp,> +static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,>                 int *valp,>                 int write, void *data)>  {> @@ -2050,7 +2190,7 @@ static int do_proc_dointvec_conv(int *ne>     } else {>         int val = *valp;>         if (val < 0) {> -            *negp = -1;> +            *negp = 1;>             *lvalp = (unsigned long)-val;>         } else {>             *negp = 0;> @@ -2060,20 +2200,18 @@ static int do_proc_dointvec_conv(int *ne>     return 0;>  }>> +static const char proc_wspace_sep[] = { ' ', '\t', '\n', 0 };> +>  static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,> -         int write, void __user *buffer,> +         int write, void __user *_buffer,>          size_t *lenp, loff_t *ppos,> -         int (*conv)(int *negp, unsigned long *lvalp, int *valp,> +         int (*conv)(bool *negp, unsigned long *lvalp, int *valp,>                int write, void *data),>          void *data)>  {> -#define TMPBUFLEN 21> -    int *i, vleft, first = 1, neg;> -    unsigned long lval;> -    size_t left, len;> -> -    char buf[TMPBUFLEN], *p;> -    char __user *s = buffer;> +    int *i, vleft, first = 1, err = 0;> +    size_t left;> +    char __user *buffer = (char __user *) _buffer;>>     if (!tbl_data || !table->maxlen || !*lenp ||>       (*ppos && !write)) {> @@ -2089,88 +2227,48 @@ static int __do_proc_dointvec(void *tbl_>         conv = do_proc_dointvec_conv;>>     for (; left && vleft--; i++, first=0) {> -        if (write) {> -            while (left) {> -                char c;> -                if (get_user(c, s))> -                    return -EFAULT;> -                if (!isspace(c))> -                    break;> -                left--;> -                s++;> -            }> -            if (!left)> -                break;> -            neg = 0;> -            len = left;> -            if (len > sizeof(buf) - 1)> -                len = sizeof(buf) - 1;> -            if (copy_from_user(buf, s, len))> -                return -EFAULT;> -            buf[len] = 0;> -            p = buf;> -            if (*p == '-' && left > 1) {> -                neg = 1;> -                p++;> -            }> -            if (*p < '0' || *p > '9')> -                break;> -> -            lval = simple_strtoul(p, &p, 0);> +        unsigned long lval;> +        bool neg;>> -            len = p-buf;> -            if ((len < left) && *p && !isspace(*p))> +        if (write) {> +            err = proc_skip_wspace(&buffer, &left);> +            if (err)> +                return err;> +            err = proc_get_ulong(&buffer, &left, &lval, &neg,> +                       proc_wspace_sep,> +                       sizeof(proc_wspace_sep), NULL);> +            if (err)>                 break;> -            s += len;> -            left -= len;> -> -            if (conv(&neg, &lval, i, 1, data))> +            if (conv(&neg, &lval, i, 1, data)) {> +                err = -EINVAL;>                 break;> +            }>         } else {> -            p = buf;> -            if (!first)> -                *p++ = '\t';> -> -            if (conv(&neg, &lval, i, 0, data))> +            if (conv(&neg, &lval, i, 0, data)) {> +                err = -EINVAL;>                 break;> -> -            sprintf(p, "%s%lu", neg ? "-" : "", lval);> -            len = strlen(buf);> -            if (len > left)> -                len = left;> -            if(copy_to_user(s, buf, len))> -                return -EFAULT;> -            left -= len;> -            s += len;> -        }> -    }> -> -    if (!write && !first && left) {> -        if(put_user('\n', s))> -            return -EFAULT;> -        left--, s++;> -    }> -    if (write) {> -        while (left) {> -            char c;> -            if (get_user(c, s++))> -                return -EFAULT;> -            if (!isspace(c))> +            }> +            err = proc_put_ulong(&buffer, &left, lval, neg, first,> +                       '\t');> +            if (err)>                 break;> -            left--;>         }>     }> +> +    if (!write && !first && left && !err)> +        err = proc_put_char(&buffer, &left, '\n');> +    if (write && !err)> +        err = proc_skip_wspace(&buffer, &left);>     if (write && first)> -        return -EINVAL;> +        return err ? : -EINVAL;>     *lenp -= left;>     *ppos += *lenp;>     return 0;> -#undef TMPBUFLEN>  }>>  static int do_proc_dointvec(struct ctl_table *table, int write,>          void __user *buffer, size_t *lenp, loff_t *ppos,> -         int (*conv)(int *negp, unsigned long *lvalp, int *valp,> +         int (*conv)(bool *negp, unsigned long *lvalp, int *valp,>                int write, void *data),>          void *data)>  {> @@ -2238,8 +2336,8 @@ struct do_proc_dointvec_minmax_conv_para>     int *max;>  };>> -static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp,> -                    int *valp,> +static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,> +                    int *valp,>                     int write, void *data)>  {>     struct do_proc_dointvec_minmax_conv_param *param = data;> @@ -2252,7 +2350,7 @@ static int do_proc_dointvec_minmax_conv(>     } else {>         int val = *valp;>         if (val < 0) {> -            *negp = -1;> +            *negp = 1;>             *lvalp = (unsigned long)-val;>         } else {>             *negp = 0;> @@ -2290,17 +2388,15 @@ int proc_dointvec_minmax(struct ctl_tabl>  }>>  static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write,> -                   void __user *buffer,> +                   void __user *_buffer,>                   size_t *lenp, loff_t *ppos,>                   unsigned long convmul,>                   unsigned long convdiv)>  {> -#define TMPBUFLEN 21> -    unsigned long *i, *min, *max, val;> -    int vleft, first=1, neg;> -    size_t len, left;> -    char buf[TMPBUFLEN], *p;> -    char __user *s = buffer;> +    unsigned long *i, *min, *max;> +    int vleft, first = 1, err = 0;> +    size_t left;> +    char __user *buffer = (char __user *) _buffer;>>     if (!data || !table->maxlen || !*lenp ||>       (*ppos && !write)) {> @@ -2315,82 +2411,42 @@ static int __do_proc_doulongvec_minmax(v>     left = *lenp;>>     for (; left && vleft--; i++, min++, max++, first=0) {> +        unsigned long val;> +>         if (write) {> -            while (left) {> -                char c;> -                if (get_user(c, s))> -                    return -EFAULT;> -                if (!isspace(c))> -                    break;> -                left--;> -                s++;> -            }> -            if (!left)> -                break;> -            neg = 0;> -            len = left;> -            if (len > TMPBUFLEN-1)> -                len = TMPBUFLEN-1;> -            if (copy_from_user(buf, s, len))> -                return -EFAULT;> -            buf[len] = 0;> -            p = buf;> -            if (*p == '-' && left > 1) {> -                neg = 1;> -                p++;> -            }> -            if (*p < '0' || *p > '9')> -                break;> -            val = simple_strtoul(p, &p, 0) * convmul / convdiv ;> -            len = p-buf;> -            if ((len < left) && *p && !isspace(*p))> +            bool neg;> +> +            err = proc_skip_wspace(&buffer, &left);> +            if (err)> +                return err;> +            err = proc_get_ulong(&buffer, &left, &val, &neg,> +                       proc_wspace_sep,> +                       sizeof(proc_wspace_sep), NULL);> +            if (err)>                 break;>             if (neg)> -                val = -val;> -            s += len;> -            left -= len;> -> -            if(neg)>                 continue;>             if ((min && val < *min) || (max && val > *max))>                 continue;>             *i = val;>         } else {> -            p = buf;> -            if (!first)> -                *p++ = '\t';> -            sprintf(p, "%lu", convdiv * (*i) / convmul);> -            len = strlen(buf);> -            if (len > left)> -                len = left;> -            if(copy_to_user(s, buf, len))> -                return -EFAULT;> -            left -= len;> -            s += len;> -        }> -    }> -> -    if (!write && !first && left) {> -        if(put_user('\n', s))> -            return -EFAULT;> -        left--, s++;> -    }> -    if (write) {> -        while (left) {> -            char c;> -            if (get_user(c, s++))> -                return -EFAULT;> -            if (!isspace(c))> +            val = convdiv * (*i) / convmul;> +            err = proc_put_ulong(&buffer, &left, val, 0, first,> +                       '\t');> +            if (err)>                 break;> -            left--;>         }>     }> +> +    if (!write && !first && left && !err)> +        err = proc_put_char(&buffer, &left, '\n');> +    if (write && !err)> +        err = proc_skip_wspace(&buffer, &left);>     if (write && first)> -        return -EINVAL;> +        return err ? : -EINVAL;>     *lenp -= left;>     *ppos += *lenp;>     return 0;> -#undef TMPBUFLEN>  }>>  static int do_proc_doulongvec_minmax(struct ctl_table *table, int write,> @@ -2451,7 +2507,7 @@ int proc_doulongvec_ms_jiffies_minmax(st>  }>>> -static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp,> +static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp,>                     int *valp,>                     int write, void *data)>  {> @@ -2463,7 +2519,7 @@ static int do_proc_dointvec_jiffies_conv>         int val = *valp;>         unsigned long lval;>         if (val < 0) {> -            *negp = -1;> +            *negp = 1;>             lval = (unsigned long)-val;>         } else {>             *negp = 0;> @@ -2474,7 +2530,7 @@ static int do_proc_dointvec_jiffies_conv>     return 0;>  }>> -static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp,> +static int do_proc_dointvec_userhz_jiffies_conv(bool *negp, unsigned long *lvalp,>                         int *valp,>                         int write, void *data)>  {> @@ -2486,7 +2542,7 @@ static int do_proc_dointvec_userhz_jiffi>         int val = *valp;>         unsigned long lval;>         if (val < 0) {> -            *negp = -1;> +            *negp = 1;>             lval = (unsigned long)-val;>         } else {>             *negp = 0;> @@ -2497,7 +2553,7 @@ static int do_proc_dointvec_userhz_jiffi>     return 0;>  }>> -static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp,> +static int do_proc_dointvec_ms_jiffies_conv(bool *negp, unsigned long *lvalp,>                       int *valp,>                       int write, void *data)>  {> @@ -2507,7 +2563,7 @@ static int do_proc_dointvec_ms_jiffies_c>         int val = *valp;>         unsigned long lval;>         if (val < 0) {> -            *negp = -1;> +            *negp = 1;>             lval = (unsigned long)-val;>         } else {>             *negp = 0; These functions have so much lines of code. I think you can make themless. Please refer to strsep(). -- Regardsï¼Changli Gao(xiaosuo@gmail.com)ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 1/3] sysctl: refactor integer handling proc code @ 2010-04-09 10:49 ` Changli Gao 0 siblings, 0 replies; 24+ messages in thread From: Changli Gao @ 2010-04-09 10:49 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, Octavian Purdila, Eric Dumazet, netdev, Neil Horman, David Miller, ebiederm On Fri, Apr 9, 2010 at 6:11 PM, Amerigo Wang <amwang@redhat.com> wrote: > > From: Octavian Purdila <opurdila@ixiacom.com> > > As we are about to add another integer handling proc function a little > bit of cleanup is in order: add a few helper functions to improve code > readability and decrease code duplication. > > In the process a bug is also fixed: if the user specifies a number > with more then 20 digits it will be interpreted as two integers > (e.g. 10000...13 will be interpreted as 100.... and 13). > > Behavior for EFAULT handling was changed as well. Previous to this > patch, when an EFAULT error occurred in the middle of a write > operation, although some of the elements were set, that was not > acknowledged to the user (by shorting the write and returning the > number of bytes accepted). EFAULT is now treated just like any other > errors by acknowledging the amount of bytes accepted. > > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> > Signed-off-by: WANG Cong <amwang@redhat.com> > Cc: Eric W. Biederman <ebiederm@xmission.com> > --- > > Index: linux-2.6/kernel/sysctl.c > =================================================================== > --- linux-2.6.orig/kernel/sysctl.c > +++ linux-2.6/kernel/sysctl.c > @@ -2040,8 +2040,148 @@ int proc_dostring(struct ctl_table *tabl > buffer, lenp, ppos); > } > > +static int proc_skip_wspace(char __user **buf, size_t *size) > +{ > + char c; > + > + while (*size) { > + if (get_user(c, *buf)) > + return -EFAULT; > + if (!isspace(c)) > + break; > + (*size)--; > + (*buf)++; > + } > + > + return 0; > +} > + > +static bool isanyof(char c, const char *v, unsigned len) > +{ > + int i; > + > + if (!len) > + return false; > + > + for (i = 0; i < len; i++) > + if (c == v[i]) > + break; > + if (i == len) > + return false; > + > + return true; > +} > + > +#define TMPBUFLEN 22 > +/** > + * proc_get_ulong - reads an ASCII formated integer from a user buffer > + * > + * @buf - user buffer > + * @size - size of the user buffer > + * @val - this is where the number will be stored > + * @neg - set to %TRUE if number is negative > + * @perm_tr - a vector which contains the allowed trailers > + * @perm_tr_len - size of the perm_tr vector > + * @tr - pointer to store the trailer character > + * > + * In case of success 0 is returned and buf and size are updated with > + * the amount of bytes read. If tr is non NULL and a trailing > + * character exist (size is non zero after returning from this > + * function) tr is updated with the trailing character. > + */ > +static int proc_get_ulong(char __user **buf, size_t *size, > + unsigned long *val, bool *neg, > + const char *perm_tr, unsigned perm_tr_len, char *tr) > +{ > + int len; > + char *p, tmp[TMPBUFLEN]; > + > + if (!*size) > + return -EINVAL; > + > + len = *size; > + if (len > TMPBUFLEN-1) > + len = TMPBUFLEN-1; > + > + if (copy_from_user(tmp, *buf, len)) > + return -EFAULT; > + > + tmp[len] = 0; > + p = tmp; > + if (*p == '-' && *size > 1) { > + *neg = 1; > + p++; > + } else > + *neg = 0; the function name implies that it is used to parse unsigned long, so negative value should not be supported. > + if (!isdigit(*p)) > + return -EINVAL; It seems that ledding white space should be allowed, so this check isn't needed, and simple_strtoul can handle it. > + > + *val = simple_strtoul(p, &p, 0); > + > + len = p - tmp; > + > + /* We don't know if the next char is whitespace thus we may accept > + * invalid integers (e.g. 1234...a) or two integers instead of one > + * (e.g. 123...1). So lets not allow such large numbers. */ > + if (len == TMPBUFLEN - 1) > + return -EINVAL; > + > + if (len < *size && perm_tr_len && !isanyof(*p, perm_tr, perm_tr_len)) > + return -EINVAL; is strspn() better? > + > + if (tr && (len < *size)) > + *tr = *p; > + > + *buf += len; > + *size -= len; > + > + return 0; > +} > + > +/** > + * proc_put_ulong - coverts an integer to a decimal ASCII formated string > + * > + * @buf - the user buffer > + * @size - the size of the user buffer > + * @val - the integer to be converted > + * @neg - sign of the number, %TRUE for negative > + * @first - if %FALSE will insert a separator character before the number > + * @separator - the separator character > + * > + * In case of success 0 is returned and buf and size are updated with > + * the amount of bytes read. > + */ > +static int proc_put_ulong(char __user **buf, size_t *size, unsigned long val, > + bool neg, bool first, char separator) > +{ > + int len; > + char tmp[TMPBUFLEN], *p = tmp; > + > + if (!first) > + *p++ = separator; > + sprintf(p, "%s%lu", neg ? "-" : "", val); negative should not be supported too. > + len = strlen(tmp); > + if (len > *size) > + len = *size; > + if (copy_to_user(*buf, tmp, len)) > + return -EFAULT; > + *size -= len; > + *buf += len; > + return 0; > +} > +#undef TMPBUFLEN > + > +static int proc_put_char(char __user **buf, size_t *size, char c) > +{ > + if (*size) { > + if (put_user(c, *buf)) > + return -EFAULT; > + (*size)--, (*buf)++; > + } > + return 0; > +} > > -static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp, > +static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, > int *valp, > int write, void *data) > { > @@ -2050,7 +2190,7 @@ static int do_proc_dointvec_conv(int *ne > } else { > int val = *valp; > if (val < 0) { > - *negp = -1; > + *negp = 1; > *lvalp = (unsigned long)-val; > } else { > *negp = 0; > @@ -2060,20 +2200,18 @@ static int do_proc_dointvec_conv(int *ne > return 0; > } > > +static const char proc_wspace_sep[] = { ' ', '\t', '\n', 0 }; > + > static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table, > - int write, void __user *buffer, > + int write, void __user *_buffer, > size_t *lenp, loff_t *ppos, > - int (*conv)(int *negp, unsigned long *lvalp, int *valp, > + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, > int write, void *data), > void *data) > { > -#define TMPBUFLEN 21 > - int *i, vleft, first = 1, neg; > - unsigned long lval; > - size_t left, len; > - > - char buf[TMPBUFLEN], *p; > - char __user *s = buffer; > + int *i, vleft, first = 1, err = 0; > + size_t left; > + char __user *buffer = (char __user *) _buffer; > > if (!tbl_data || !table->maxlen || !*lenp || > (*ppos && !write)) { > @@ -2089,88 +2227,48 @@ static int __do_proc_dointvec(void *tbl_ > conv = do_proc_dointvec_conv; > > for (; left && vleft--; i++, first=0) { > - if (write) { > - while (left) { > - char c; > - if (get_user(c, s)) > - return -EFAULT; > - if (!isspace(c)) > - break; > - left--; > - s++; > - } > - if (!left) > - break; > - neg = 0; > - len = left; > - if (len > sizeof(buf) - 1) > - len = sizeof(buf) - 1; > - if (copy_from_user(buf, s, len)) > - return -EFAULT; > - buf[len] = 0; > - p = buf; > - if (*p == '-' && left > 1) { > - neg = 1; > - p++; > - } > - if (*p < '0' || *p > '9') > - break; > - > - lval = simple_strtoul(p, &p, 0); > + unsigned long lval; > + bool neg; > > - len = p-buf; > - if ((len < left) && *p && !isspace(*p)) > + if (write) { > + err = proc_skip_wspace(&buffer, &left); > + if (err) > + return err; > + err = proc_get_ulong(&buffer, &left, &lval, &neg, > + proc_wspace_sep, > + sizeof(proc_wspace_sep), NULL); > + if (err) > break; > - s += len; > - left -= len; > - > - if (conv(&neg, &lval, i, 1, data)) > + if (conv(&neg, &lval, i, 1, data)) { > + err = -EINVAL; > break; > + } > } else { > - p = buf; > - if (!first) > - *p++ = '\t'; > - > - if (conv(&neg, &lval, i, 0, data)) > + if (conv(&neg, &lval, i, 0, data)) { > + err = -EINVAL; > break; > - > - sprintf(p, "%s%lu", neg ? "-" : "", lval); > - len = strlen(buf); > - if (len > left) > - len = left; > - if(copy_to_user(s, buf, len)) > - return -EFAULT; > - left -= len; > - s += len; > - } > - } > - > - if (!write && !first && left) { > - if(put_user('\n', s)) > - return -EFAULT; > - left--, s++; > - } > - if (write) { > - while (left) { > - char c; > - if (get_user(c, s++)) > - return -EFAULT; > - if (!isspace(c)) > + } > + err = proc_put_ulong(&buffer, &left, lval, neg, first, > + '\t'); > + if (err) > break; > - left--; > } > } > + > + if (!write && !first && left && !err) > + err = proc_put_char(&buffer, &left, '\n'); > + if (write && !err) > + err = proc_skip_wspace(&buffer, &left); > if (write && first) > - return -EINVAL; > + return err ? : -EINVAL; > *lenp -= left; > *ppos += *lenp; > return 0; > -#undef TMPBUFLEN > } > > static int do_proc_dointvec(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, loff_t *ppos, > - int (*conv)(int *negp, unsigned long *lvalp, int *valp, > + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, > int write, void *data), > void *data) > { > @@ -2238,8 +2336,8 @@ struct do_proc_dointvec_minmax_conv_para > int *max; > }; > > -static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp, > - int *valp, > +static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp, > + int *valp, > int write, void *data) > { > struct do_proc_dointvec_minmax_conv_param *param = data; > @@ -2252,7 +2350,7 @@ static int do_proc_dointvec_minmax_conv( > } else { > int val = *valp; > if (val < 0) { > - *negp = -1; > + *negp = 1; > *lvalp = (unsigned long)-val; > } else { > *negp = 0; > @@ -2290,17 +2388,15 @@ int proc_dointvec_minmax(struct ctl_tabl > } > > static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write, > - void __user *buffer, > + void __user *_buffer, > size_t *lenp, loff_t *ppos, > unsigned long convmul, > unsigned long convdiv) > { > -#define TMPBUFLEN 21 > - unsigned long *i, *min, *max, val; > - int vleft, first=1, neg; > - size_t len, left; > - char buf[TMPBUFLEN], *p; > - char __user *s = buffer; > + unsigned long *i, *min, *max; > + int vleft, first = 1, err = 0; > + size_t left; > + char __user *buffer = (char __user *) _buffer; > > if (!data || !table->maxlen || !*lenp || > (*ppos && !write)) { > @@ -2315,82 +2411,42 @@ static int __do_proc_doulongvec_minmax(v > left = *lenp; > > for (; left && vleft--; i++, min++, max++, first=0) { > + unsigned long val; > + > if (write) { > - while (left) { > - char c; > - if (get_user(c, s)) > - return -EFAULT; > - if (!isspace(c)) > - break; > - left--; > - s++; > - } > - if (!left) > - break; > - neg = 0; > - len = left; > - if (len > TMPBUFLEN-1) > - len = TMPBUFLEN-1; > - if (copy_from_user(buf, s, len)) > - return -EFAULT; > - buf[len] = 0; > - p = buf; > - if (*p == '-' && left > 1) { > - neg = 1; > - p++; > - } > - if (*p < '0' || *p > '9') > - break; > - val = simple_strtoul(p, &p, 0) * convmul / convdiv ; > - len = p-buf; > - if ((len < left) && *p && !isspace(*p)) > + bool neg; > + > + err = proc_skip_wspace(&buffer, &left); > + if (err) > + return err; > + err = proc_get_ulong(&buffer, &left, &val, &neg, > + proc_wspace_sep, > + sizeof(proc_wspace_sep), NULL); > + if (err) > break; > if (neg) > - val = -val; > - s += len; > - left -= len; > - > - if(neg) > continue; > if ((min && val < *min) || (max && val > *max)) > continue; > *i = val; > } else { > - p = buf; > - if (!first) > - *p++ = '\t'; > - sprintf(p, "%lu", convdiv * (*i) / convmul); > - len = strlen(buf); > - if (len > left) > - len = left; > - if(copy_to_user(s, buf, len)) > - return -EFAULT; > - left -= len; > - s += len; > - } > - } > - > - if (!write && !first && left) { > - if(put_user('\n', s)) > - return -EFAULT; > - left--, s++; > - } > - if (write) { > - while (left) { > - char c; > - if (get_user(c, s++)) > - return -EFAULT; > - if (!isspace(c)) > + val = convdiv * (*i) / convmul; > + err = proc_put_ulong(&buffer, &left, val, 0, first, > + '\t'); > + if (err) > break; > - left--; > } > } > + > + if (!write && !first && left && !err) > + err = proc_put_char(&buffer, &left, '\n'); > + if (write && !err) > + err = proc_skip_wspace(&buffer, &left); > if (write && first) > - return -EINVAL; > + return err ? : -EINVAL; > *lenp -= left; > *ppos += *lenp; > return 0; > -#undef TMPBUFLEN > } > > static int do_proc_doulongvec_minmax(struct ctl_table *table, int write, > @@ -2451,7 +2507,7 @@ int proc_doulongvec_ms_jiffies_minmax(st > } > > > -static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp, > +static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp, > int *valp, > int write, void *data) > { > @@ -2463,7 +2519,7 @@ static int do_proc_dointvec_jiffies_conv > int val = *valp; > unsigned long lval; > if (val < 0) { > - *negp = -1; > + *negp = 1; > lval = (unsigned long)-val; > } else { > *negp = 0; > @@ -2474,7 +2530,7 @@ static int do_proc_dointvec_jiffies_conv > return 0; > } > > -static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp, > +static int do_proc_dointvec_userhz_jiffies_conv(bool *negp, unsigned long *lvalp, > int *valp, > int write, void *data) > { > @@ -2486,7 +2542,7 @@ static int do_proc_dointvec_userhz_jiffi > int val = *valp; > unsigned long lval; > if (val < 0) { > - *negp = -1; > + *negp = 1; > lval = (unsigned long)-val; > } else { > *negp = 0; > @@ -2497,7 +2553,7 @@ static int do_proc_dointvec_userhz_jiffi > return 0; > } > > -static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp, > +static int do_proc_dointvec_ms_jiffies_conv(bool *negp, unsigned long *lvalp, > int *valp, > int write, void *data) > { > @@ -2507,7 +2563,7 @@ static int do_proc_dointvec_ms_jiffies_c > int val = *valp; > unsigned long lval; > if (val < 0) { > - *negp = -1; > + *negp = 1; > lval = (unsigned long)-val; > } else { > *negp = 0; These functions have so much lines of code. I think you can make them less. Please refer to strsep(). -- Regards, Changli Gao(xiaosuo@gmail.com) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 1/3] sysctl: refactor integer handling proc code 2010-04-09 10:49 ` Changli Gao (?) @ 2010-04-09 13:40 ` Octavian Purdila 2010-04-12 6:30 ` Cong Wang -1 siblings, 1 reply; 24+ messages in thread From: Octavian Purdila @ 2010-04-09 13:40 UTC (permalink / raw) To: Changli Gao Cc: Amerigo Wang, linux-kernel, Eric Dumazet, netdev, Neil Horman, David Miller, ebiederm Hi and thanks for reviewing. On Friday 09 April 2010 13:49:12 you wrote: > > + * > > + * In case of success 0 is returned and buf and size are updated with > > + * the amount of bytes read. If tr is non NULL and a trailing > > + * character exist (size is non zero after returning from this > > + * function) tr is updated with the trailing character. > > + */ > > +static int proc_get_ulong(char __user **buf, size_t *size, > > + unsigned long *val, bool *neg, > > + const char *perm_tr, unsigned perm_tr_len, char > > *tr) +{ > > + int len; > > + char *p, tmp[TMPBUFLEN]; > > + > > + if (!*size) > > + return -EINVAL; > > + > > + len = *size; > > + if (len > TMPBUFLEN-1) > > + len = TMPBUFLEN-1; > > + > > + if (copy_from_user(tmp, *buf, len)) > > + return -EFAULT; > > + > > + tmp[len] = 0; > > + p = tmp; > > + if (*p == '-' && *size > 1) { > > + *neg = 1; > > + p++; > > + } else > > + *neg = 0; > > the function name implies that it is used to parse unsigned long, so > negative value should not be supported. > My intention was to signal that the argument is unsigned long and that the sign come separate in neg, but I am OK with changing the function name to proc_get_long() if you think that is better. > > + if (!isdigit(*p)) > > + return -EINVAL; > > It seems that ledding white space should be allowed, so this check > isn't needed, and simple_strtoul can handle it. > Leading white space is skipped with proc_skip_space before calling this function. AFAICS simple_strtoul does not handle whitespaces. > > + > > + *val = simple_strtoul(p, &p, 0); > > + > > + len = p - tmp; > > + > > + /* We don't know if the next char is whitespace thus we may > > accept + * invalid integers (e.g. 1234...a) or two integers > > instead of one + * (e.g. 123...1). So lets not allow such large > > numbers. */ + if (len == TMPBUFLEN - 1) > > + return -EINVAL; > > + > > + if (len < *size && perm_tr_len && !isanyof(*p, perm_tr, > > perm_tr_len)) + return -EINVAL; > > is strspn() better? > I don't think it will work out, \0 is an accepted trailer for many of the function which use this function. > > + > > + if (tr && (len < *size)) > > + *tr = *p; > > + > > + *buf += len; > > + *size -= len; > > + > > + return 0; > > +} > > + > > +/** > > + * proc_put_ulong - coverts an integer to a decimal ASCII formated > > string + * > > + * @buf - the user buffer > > + * @size - the size of the user buffer > > + * @val - the integer to be converted > > + * @neg - sign of the number, %TRUE for negative > > + * @first - if %FALSE will insert a separator character before the > > number + * @separator - the separator character > > + * > > + * In case of success 0 is returned and buf and size are updated with > > + * the amount of bytes read. > > + */ > > +static int proc_put_ulong(char __user **buf, size_t *size, unsigned long > > val, + bool neg, bool first, char separator) > > +{ > > + int len; > > + char tmp[TMPBUFLEN], *p = tmp; > > + > > + if (!first) > > + *p++ = separator; > > + sprintf(p, "%s%lu", neg ? "-" : "", val); > > negative should not be supported too. > We need negatives in proc_dointvec, again we can change the function name if it will clear things up. <snip> > > int val = *valp; > > unsigned long lval; > > if (val < 0) { > > - *negp = -1; > > + *negp = 1; > > lval = (unsigned long)-val; > > } else { > > *negp = 0; > > These functions have so much lines of code. I think you can make them > less. Please refer to strsep(). > Hmm, the input its pretty permissive and maybe this is why it looks so fat, we need to account for quite a few cases. Or maybe I spent too much time on this code already and I can't see the simple solution :) Thanks, tavi ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 1/3] sysctl: refactor integer handling proc code 2010-04-09 13:40 ` Octavian Purdila @ 2010-04-12 6:30 ` Cong Wang 0 siblings, 0 replies; 24+ messages in thread From: Cong Wang @ 2010-04-12 6:30 UTC (permalink / raw) To: Octavian Purdila Cc: Changli Gao, linux-kernel, Eric Dumazet, netdev, Neil Horman, David Miller, ebiederm Octavian Purdila wrote: > Hi and thanks for reviewing. > > On Friday 09 April 2010 13:49:12 you wrote: >>> + * >>> + * In case of success 0 is returned and buf and size are updated with >>> + * the amount of bytes read. If tr is non NULL and a trailing >>> + * character exist (size is non zero after returning from this >>> + * function) tr is updated with the trailing character. >>> + */ >>> +static int proc_get_ulong(char __user **buf, size_t *size, >>> + unsigned long *val, bool *neg, >>> + const char *perm_tr, unsigned perm_tr_len, char >>> *tr) +{ >>> + int len; >>> + char *p, tmp[TMPBUFLEN]; >>> + >>> + if (!*size) >>> + return -EINVAL; >>> + >>> + len = *size; >>> + if (len > TMPBUFLEN-1) >>> + len = TMPBUFLEN-1; >>> + >>> + if (copy_from_user(tmp, *buf, len)) >>> + return -EFAULT; >>> + >>> + tmp[len] = 0; >>> + p = tmp; >>> + if (*p == '-' && *size > 1) { >>> + *neg = 1; >>> + p++; >>> + } else >>> + *neg = 0; >> the function name implies that it is used to parse unsigned long, so >> negative value should not be supported. >> > > My intention was to signal that the argument is unsigned long and that the > sign come separate in neg, but I am OK with changing the function name to > proc_get_long() if you think that is better. > >>> + if (!isdigit(*p)) >>> + return -EINVAL; >> It seems that ledding white space should be allowed, so this check >> isn't needed, and simple_strtoul can handle it. >> > > Leading white space is skipped with proc_skip_space before calling this > function. AFAICS simple_strtoul does not handle whitespaces. Right, callers already skip spaces. > >>> + >>> + *val = simple_strtoul(p, &p, 0); >>> + >>> + len = p - tmp; >>> + >>> + /* We don't know if the next char is whitespace thus we may >>> accept + * invalid integers (e.g. 1234...a) or two integers >>> instead of one + * (e.g. 123...1). So lets not allow such large >>> numbers. */ + if (len == TMPBUFLEN - 1) >>> + return -EINVAL; >>> + >>> + if (len < *size && perm_tr_len && !isanyof(*p, perm_tr, >>> perm_tr_len)) + return -EINVAL; >> is strspn() better? >> > > I don't think it will work out, \0 is an accepted trailer for many of the > function which use this function. > Yes, that is why 'len' is the last parameter of isanyof(). > >>> + >>> + if (tr && (len < *size)) >>> + *tr = *p; >>> + >>> + *buf += len; >>> + *size -= len; >>> + >>> + return 0; >>> +} >>> + >>> +/** >>> + * proc_put_ulong - coverts an integer to a decimal ASCII formated >>> string + * >>> + * @buf - the user buffer >>> + * @size - the size of the user buffer >>> + * @val - the integer to be converted >>> + * @neg - sign of the number, %TRUE for negative >>> + * @first - if %FALSE will insert a separator character before the >>> number + * @separator - the separator character >>> + * >>> + * In case of success 0 is returned and buf and size are updated with >>> + * the amount of bytes read. >>> + */ >>> +static int proc_put_ulong(char __user **buf, size_t *size, unsigned long >>> val, + bool neg, bool first, char separator) >>> +{ >>> + int len; >>> + char tmp[TMPBUFLEN], *p = tmp; >>> + >>> + if (!first) >>> + *p++ = separator; >>> + sprintf(p, "%s%lu", neg ? "-" : "", val); >> negative should not be supported too. >> > > We need negatives in proc_dointvec, again we can change the function name if > it will clear things up. > Yeah, I will change them to proc_{get,put}_long(). > <snip> >>> int val = *valp; >>> unsigned long lval; >>> if (val < 0) { >>> - *negp = -1; >>> + *negp = 1; >>> lval = (unsigned long)-val; >>> } else { >>> *negp = 0; >> These functions have so much lines of code. I think you can make them >> less. Please refer to strsep(). >> > > Hmm, the input its pretty permissive and maybe this is why it looks so fat, we > need to account for quite a few cases. > Because the original code is messy and already has much lines. Thanks. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Patch 2/3] sysctl: add proc_do_large_bitmap 2010-04-09 10:10 [Patch v7 0/3] net: reserve ports for applications using fixed port numbers Amerigo Wang 2010-04-09 10:11 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang @ 2010-04-09 10:11 ` Amerigo Wang 2010-04-09 10:33 ` Changli Gao 2010-04-09 10:11 ` [Patch 3/3] net: reserve ports for applications using fixed port numbers Amerigo Wang 2 siblings, 1 reply; 24+ messages in thread From: Amerigo Wang @ 2010-04-09 10:11 UTC (permalink / raw) To: linux-kernel Cc: Octavian Purdila, ebiederm, Eric Dumazet, netdev, Neil Horman, Amerigo Wang, David Miller From: Octavian Purdila <opurdila@ixiacom.com> The new function can be used to read/write large bitmaps via /proc. A comma separated range format is used for compact output and input (e.g. 1,3-4,10-10). Writing into the file will first reset the bitmap then update it based on the given input. Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> Signed-off-by: WANG Cong <amwang@redhat.com> Cc: Eric W. Biederman <ebiederm@xmission.com> --- Index: linux-2.6/include/linux/sysctl.h =================================================================== --- linux-2.6.orig/include/linux/sysctl.h +++ linux-2.6/include/linux/sysctl.h @@ -980,6 +980,8 @@ extern int proc_doulongvec_minmax(struct void __user *, size_t *, loff_t *); extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int, void __user *, size_t *, loff_t *); +extern int proc_do_large_bitmap(struct ctl_table *, int, + void __user *, size_t *, loff_t *); /* * Register a set of sysctl names by calling register_sysctl_table Index: linux-2.6/kernel/sysctl.c =================================================================== --- linux-2.6.orig/kernel/sysctl.c +++ linux-2.6/kernel/sysctl.c @@ -2072,6 +2072,23 @@ static bool isanyof(char c, const char * return true; } +static int proc_skip_anyof(char __user **buf, size_t *size, + const char *v, unsigned len) +{ + char c; + + while (*size) { + if (get_user(c, *buf)) + return -EFAULT; + if (!isanyof(c, v, len)) + break; + (*size)--; + (*buf)++; + } + + return 0; +} + #define TMPBUFLEN 22 /** * proc_get_ulong - reads an ASCII formated integer from a user buffer @@ -2663,6 +2680,135 @@ static int proc_do_cad_pid(struct ctl_ta return 0; } +/** + * proc_do_large_bitmap - read/write from/to a large bitmap + * @table: the sysctl table + * @write: %TRUE if this is a write to the sysctl file + * @buffer: the user buffer + * @lenp: the size of the user buffer + * @ppos: file position + * + * The bitmap is stored at table->data and the bitmap length (in bits) + * in table->maxlen. + * + * We use a range comma separated format (e.g. 1,3-4,10-10) so that + * large bitmaps may be represented in a compact manner. Writing into + * the file will clear the bitmap then update it with the given input. + * + * Returns 0 on success. + */ +int proc_do_large_bitmap(struct ctl_table *table, int write, + void __user *_buffer, size_t *lenp, loff_t *ppos) +{ + int err = 0; + bool first = 1; + size_t left = *lenp; + unsigned long bitmap_len = table->maxlen; + char __user *buffer = (char __user *) _buffer; + unsigned long *bitmap = (unsigned long *) table->data; + unsigned long *tmp_bitmap = NULL; + char tr_a[] = { '-', ',', '\n', 0 }, tr_b[] = { ',', '\n', 0 }, c; + char tr_end[] = { '\n', 0 }; + + + if (!bitmap_len || !left || (*ppos && !write)) { + *lenp = 0; + return 0; + } + + if (write) { + tmp_bitmap = kzalloc(BITS_TO_LONGS(bitmap_len) * sizeof(unsigned long), + GFP_KERNEL); + if (!tmp_bitmap) + return -ENOMEM; + err = proc_skip_anyof(&buffer, &left, tr_end, sizeof(tr_end)); + while (!err && left) { + unsigned long val_a, val_b; + bool neg; + + err = proc_get_ulong(&buffer, &left, &val_a, &neg, tr_a, + sizeof(tr_a), &c); + if (err) + break; + if (val_a >= bitmap_len || neg) { + err = -EINVAL; + break; + } + + val_b = val_a; + if (left) { + buffer++; + left--; + } + + if (c == '-') { + err = proc_get_ulong(&buffer, &left, &val_b, + &neg, tr_b, sizeof(tr_b), + &c); + if (err) + break; + if (val_b >= bitmap_len || neg || + val_a > val_b) { + err = -EINVAL; + break; + } + if (left) { + buffer++; + left--; + } + } + + while (val_a <= val_b) + set_bit(val_a++, tmp_bitmap); + + first = 0; + err = proc_skip_anyof(&buffer, &left, tr_end, + sizeof(tr_end)); + } + } else { + unsigned long bit_a, bit_b = 0; + + while (left) { + bit_a = find_next_bit(bitmap, bitmap_len, bit_b); + if (bit_a >= bitmap_len) + break; + bit_b = find_next_zero_bit(bitmap, bitmap_len, + bit_a + 1) - 1; + + err = proc_put_ulong(&buffer, &left, bit_a, 0, first, + ','); + if (err) + break; + if (bit_a != bit_b) { + err = proc_put_char(&buffer, &left, '-'); + if (err) + break; + err = proc_put_ulong(&buffer, &left, bit_b, 0, + 1, 0); + if (err) + break; + } + + first = 0; bit_b++; + } + if (!err) + err = proc_put_char(&buffer, &left, '\n'); + } + + if (!err) { + if (write) + memcpy(bitmap, tmp_bitmap, + BITS_TO_LONGS(bitmap_len) * sizeof(unsigned long)); + kfree(tmp_bitmap); + *lenp -= left; + *ppos += *lenp; + return 0; + } else { + kfree(tmp_bitmap); + return err; + } +} + #else /* CONFIG_PROC_FS */ int proc_dostring(struct ctl_table *table, int write, ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 2/3] sysctl: add proc_do_large_bitmap 2010-04-09 10:11 ` [Patch 2/3] sysctl: add proc_do_large_bitmap Amerigo Wang @ 2010-04-09 10:33 ` Changli Gao 2010-04-09 12:35 ` Octavian Purdila 2010-04-12 6:32 ` Cong Wang 0 siblings, 2 replies; 24+ messages in thread From: Changli Gao @ 2010-04-09 10:33 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, Octavian Purdila, ebiederm, Eric Dumazet, netdev, Neil Horman, David Miller On Fri, Apr 9, 2010 at 6:11 PM, Amerigo Wang <amwang@redhat.com> wrote: > From: Octavian Purdila <opurdila@ixiacom.com> > > The new function can be used to read/write large bitmaps via /proc. A > comma separated range format is used for compact output and input > (e.g. 1,3-4,10-10). > > Writing into the file will first reset the bitmap then update it > based on the given input. > We have bitmap_scnprintf() and bitmap_parse_user(), why invent a new suite? -- Regards, Changli Gao(xiaosuo@gmail.com) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 2/3] sysctl: add proc_do_large_bitmap 2010-04-09 10:33 ` Changli Gao @ 2010-04-09 12:35 ` Octavian Purdila 2010-04-12 6:32 ` Cong Wang 1 sibling, 0 replies; 24+ messages in thread From: Octavian Purdila @ 2010-04-09 12:35 UTC (permalink / raw) To: Changli Gao Cc: Amerigo Wang, linux-kernel, ebiederm, Eric Dumazet, netdev, Neil Horman, David Miller On Friday 09 April 2010 13:33:29 you wrote: > On Fri, Apr 9, 2010 at 6:11 PM, Amerigo Wang <amwang@redhat.com> wrote: > > From: Octavian Purdila <opurdila@ixiacom.com> > > > > The new function can be used to read/write large bitmaps via /proc. A > > comma separated range format is used for compact output and input > > (e.g. 1,3-4,10-10). > > > > Writing into the file will first reset the bitmap then update it > > based on the given input. > > We have bitmap_scnprintf() and bitmap_parse_user(), why invent a new suite? > A decimal comma separated ranges seems the best option for this feature, and unfortunately both of the above functions only support hexadecimal and no ranges. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 2/3] sysctl: add proc_do_large_bitmap 2010-04-09 10:33 ` Changli Gao 2010-04-09 12:35 ` Octavian Purdila @ 2010-04-12 6:32 ` Cong Wang 1 sibling, 0 replies; 24+ messages in thread From: Cong Wang @ 2010-04-12 6:32 UTC (permalink / raw) To: Changli Gao Cc: linux-kernel, Octavian Purdila, ebiederm, Eric Dumazet, netdev, Neil Horman, David Miller Changli Gao wrote: > On Fri, Apr 9, 2010 at 6:11 PM, Amerigo Wang <amwang@redhat.com> wrote: >> From: Octavian Purdila <opurdila@ixiacom.com> >> >> The new function can be used to read/write large bitmaps via /proc. A >> comma separated range format is used for compact output and input >> (e.g. 1,3-4,10-10). >> >> Writing into the file will first reset the bitmap then update it >> based on the given input. >> > > We have bitmap_scnprintf() and bitmap_parse_user(), why invent a new suite? > > Because they don't accept '-'. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Patch 3/3] net: reserve ports for applications using fixed port numbers 2010-04-09 10:10 [Patch v7 0/3] net: reserve ports for applications using fixed port numbers Amerigo Wang 2010-04-09 10:11 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang 2010-04-09 10:11 ` [Patch 2/3] sysctl: add proc_do_large_bitmap Amerigo Wang @ 2010-04-09 10:11 ` Amerigo Wang 2010-04-09 13:21 ` Tetsuo Handa 2 siblings, 1 reply; 24+ messages in thread From: Amerigo Wang @ 2010-04-09 10:11 UTC (permalink / raw) To: linux-kernel Cc: Octavian Purdila, Eric Dumazet, netdev, Neil Horman, Amerigo Wang, David Miller, ebiederm From: Octavian Purdila <opurdila@ixiacom.com> This patch introduces /proc/sys/net/ipv4/ip_local_reserved_ports which allows users to reserve ports for third-party applications. The reserved ports will not be used by automatic port assignments (e.g. when calling connect() or bind() with port number 0). Explicit port allocation behavior is unchanged. Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> Signed-off-by: WANG Cong <amwang@redhat.com> Cc: Neil Horman <nhorman@tuxdriver.com> Cc: Eric Dumazet <eric.dumazet@gmail.com> Cc: Eric W. Biederman <ebiederm@xmission.com> --- Index: linux-2.6/Documentation/networking/ip-sysctl.txt =================================================================== --- linux-2.6.orig/Documentation/networking/ip-sysctl.txt +++ linux-2.6/Documentation/networking/ip-sysctl.txt @@ -588,6 +588,37 @@ ip_local_port_range - 2 INTEGERS (i.e. by default) range 1024-4999 is enough to issue up to 2000 connections per second to systems supporting timestamps. +ip_local_reserved_ports - list of comma separated ranges + Specify the ports which are reserved for known third-party + applications. These ports will not be used by automatic port + assignments (e.g. when calling connect() or bind() with port + number 0). Explicit port allocation behavior is unchanged. + + The format used for both input and output is a comma separated + list of ranges (e.g. "1,2-4,10-10" for ports 1, 2, 3, 4 and + 10). Writing to the file will clear all previously reserved + ports and update the current list with the one given in the + input. + + Note that ip_local_port_range and ip_local_reserved_ports + settings are independent and both are considered by the kernel + when determining which ports are available for automatic port + assignments. + + You can reserve ports which are not in the current + ip_local_port_range, e.g.: + + $ cat /proc/sys/net/ipv4/ip_local_port_range + 32000 61000 + $ cat /proc/sys/net/ipv4/ip_local_reserved_ports + 8080,9148 + + although this is redundant. However such a setting is useful + if later the port range is changed to a value that will + include the reserved ports. + + Default: Empty + ip_nonlocal_bind - BOOLEAN If set, allows processes to bind() to non-local IP addresses, which can be quite useful - but may break some applications. Index: linux-2.6/drivers/infiniband/core/cma.c =================================================================== --- linux-2.6.orig/drivers/infiniband/core/cma.c +++ linux-2.6/drivers/infiniband/core/cma.c @@ -1980,6 +1980,8 @@ retry: /* FIXME: add proper port randomization per like inet_csk_get_port */ do { ret = idr_get_new_above(ps, bind_list, next_port, &port); + if (inet_is_reserved_local_port(port)) + ret = -EAGAIN; } while ((ret == -EAGAIN) && idr_pre_get(ps, GFP_KERNEL)); if (ret) @@ -2996,10 +2998,13 @@ static int __init cma_init(void) { int ret, low, high, remaining; - get_random_bytes(&next_port, sizeof next_port); inet_get_local_port_range(&low, &high); +again: + get_random_bytes(&next_port, sizeof next_port); remaining = (high - low) + 1; next_port = ((unsigned int) next_port % remaining) + low; + if (inet_is_reserved_local_port(next_port)) + goto again; cma_wq = create_singlethread_workqueue("rdma_cm"); if (!cma_wq) Index: linux-2.6/include/net/ip.h =================================================================== --- linux-2.6.orig/include/net/ip.h +++ linux-2.6/include/net/ip.h @@ -184,6 +184,12 @@ extern struct local_ports { } sysctl_local_ports; extern void inet_get_local_port_range(int *low, int *high); +extern unsigned long *sysctl_local_reserved_ports; +static inline int inet_is_reserved_local_port(int port) +{ + return test_bit(port, sysctl_local_reserved_ports); +} + extern int sysctl_ip_default_ttl; extern int sysctl_ip_nonlocal_bind; Index: linux-2.6/net/ipv4/af_inet.c =================================================================== --- linux-2.6.orig/net/ipv4/af_inet.c +++ linux-2.6/net/ipv4/af_inet.c @@ -1552,9 +1552,13 @@ static int __init inet_init(void) BUILD_BUG_ON(sizeof(struct inet_skb_parm) > sizeof(dummy_skb->cb)); + sysctl_local_reserved_ports = kzalloc(65536 / 8, GFP_KERNEL); + if (!sysctl_local_reserved_ports) + goto out; + rc = proto_register(&tcp_prot, 1); if (rc) - goto out; + goto out_free_reserved_ports; rc = proto_register(&udp_prot, 1); if (rc) @@ -1653,6 +1657,8 @@ out_unregister_udp_proto: proto_unregister(&udp_prot); out_unregister_tcp_proto: proto_unregister(&tcp_prot); +out_free_reserved_ports: + kfree(sysctl_local_reserved_ports); goto out; } Index: linux-2.6/net/ipv4/inet_connection_sock.c =================================================================== --- linux-2.6.orig/net/ipv4/inet_connection_sock.c +++ linux-2.6/net/ipv4/inet_connection_sock.c @@ -37,6 +37,9 @@ struct local_ports sysctl_local_ports __ .range = { 32768, 61000 }, }; +unsigned long *sysctl_local_reserved_ports; +EXPORT_SYMBOL(sysctl_local_reserved_ports); + void inet_get_local_port_range(int *low, int *high) { unsigned seq; @@ -108,6 +111,8 @@ again: smallest_size = -1; do { + if (inet_is_reserved_local_port(rover)) + goto next_nolock; head = &hashinfo->bhash[inet_bhashfn(net, rover, hashinfo->bhash_size)]; spin_lock(&head->lock); @@ -130,6 +135,7 @@ again: break; next: spin_unlock(&head->lock); + next_nolock: if (++rover > high) rover = low; } while (--remaining > 0); Index: linux-2.6/net/ipv4/inet_hashtables.c =================================================================== --- linux-2.6.orig/net/ipv4/inet_hashtables.c +++ linux-2.6/net/ipv4/inet_hashtables.c @@ -456,6 +456,8 @@ int __inet_hash_connect(struct inet_time local_bh_disable(); for (i = 1; i <= remaining; i++) { port = low + (i + offset) % remaining; + if (inet_is_reserved_local_port(port)) + continue; head = &hinfo->bhash[inet_bhashfn(net, port, hinfo->bhash_size)]; spin_lock(&head->lock); Index: linux-2.6/net/ipv4/sysctl_net_ipv4.c =================================================================== --- linux-2.6.orig/net/ipv4/sysctl_net_ipv4.c +++ linux-2.6/net/ipv4/sysctl_net_ipv4.c @@ -299,6 +299,13 @@ static struct ctl_table ipv4_table[] = { .mode = 0644, .proc_handler = ipv4_local_port_range, }, + { + .procname = "ip_local_reserved_ports", + .data = NULL, /* initialized in sysctl_ipv4_init */ + .maxlen = 65536, + .mode = 0644, + .proc_handler = proc_do_large_bitmap, + }, #ifdef CONFIG_IP_MULTICAST { .procname = "igmp_max_memberships", @@ -736,6 +743,16 @@ static __net_initdata struct pernet_oper static __init int sysctl_ipv4_init(void) { struct ctl_table_header *hdr; + struct ctl_table *i; + + for (i = ipv4_table; i->procname; i++) { + if (strcmp(i->procname, "ip_local_reserved_ports") == 0) { + i->data = sysctl_local_reserved_ports; + break; + } + } + if (!i->procname) + return -EINVAL; hdr = register_sysctl_paths(net_ipv4_ctl_path, ipv4_table); if (hdr == NULL) Index: linux-2.6/net/ipv4/udp.c =================================================================== --- linux-2.6.orig/net/ipv4/udp.c +++ linux-2.6/net/ipv4/udp.c @@ -233,7 +233,8 @@ int udp_lib_get_port(struct sock *sk, un */ do { if (low <= snum && snum <= high && - !test_bit(snum >> udptable->log, bitmap)) + !test_bit(snum >> udptable->log, bitmap) && + !inet_is_reserved_local_port(snum)) goto found; snum += rand; } while (snum != first); Index: linux-2.6/net/sctp/socket.c =================================================================== --- linux-2.6.orig/net/sctp/socket.c +++ linux-2.6/net/sctp/socket.c @@ -5436,6 +5436,8 @@ static long sctp_get_port_local(struct s rover++; if ((rover < low) || (rover > high)) rover = low; + if (inet_is_reserved_local_port(rover)) + continue; index = sctp_phashfn(rover); head = &sctp_port_hashtable[index]; sctp_spin_lock(&head->lock); ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 3/3] net: reserve ports for applications using fixed port numbers 2010-04-09 10:11 ` [Patch 3/3] net: reserve ports for applications using fixed port numbers Amerigo Wang @ 2010-04-09 13:21 ` Tetsuo Handa 2010-04-12 6:52 ` Cong Wang 0 siblings, 1 reply; 24+ messages in thread From: Tetsuo Handa @ 2010-04-09 13:21 UTC (permalink / raw) To: amwang, linux-kernel Cc: opurdila, eric.dumazet, netdev, nhorman, davem, ebiederm Hello. Amerigo Wang wrote: > Index: linux-2.6/drivers/infiniband/core/cma.c > =================================================================== > --- linux-2.6.orig/drivers/infiniband/core/cma.c > +++ linux-2.6/drivers/infiniband/core/cma.c > @@ -1980,6 +1980,8 @@ retry: > /* FIXME: add proper port randomization per like inet_csk_get_port */ > do { > ret = idr_get_new_above(ps, bind_list, next_port, &port); > + if (inet_is_reserved_local_port(port)) > + ret = -EAGAIN; You should not overwrite ret with -EAGAIN when idr_get_new_above() returned -ENOSPC. I don't know about idr, thus I don't know whether if (!ret && inet_is_reserved_local_port(port)) ret = -EAGAIN; is correct or not. > } while ((ret == -EAGAIN) && idr_pre_get(ps, GFP_KERNEL)); > > if (ret) > @@ -2996,10 +2998,13 @@ static int __init cma_init(void) > { > int ret, low, high, remaining; > > - get_random_bytes(&next_port, sizeof next_port); > inet_get_local_port_range(&low, &high); > +again: > + get_random_bytes(&next_port, sizeof next_port); > remaining = (high - low) + 1; > next_port = ((unsigned int) next_port % remaining) + low; > + if (inet_is_reserved_local_port(next_port)) > + goto again; > You should not unconditionally "goto again;". If all ports were reserved, it will loop forever (CPU stalls). > cma_wq = create_singlethread_workqueue("rdma_cm"); > if (!cma_wq) > Index: linux-2.6/net/sctp/socket.c > =================================================================== > --- linux-2.6.orig/net/sctp/socket.c > +++ linux-2.6/net/sctp/socket.c > @@ -5436,6 +5436,8 @@ static long sctp_get_port_local(struct s > rover++; > if ((rover < low) || (rover > high)) > rover = low; > + if (inet_is_reserved_local_port(rover)) > + continue; This one needs to be if (inet_is_reserved_local_port(rover)) goto next_nolock; > index = sctp_phashfn(rover); > head = &sctp_port_hashtable[index]; > sctp_spin_lock(&head->lock); next: sctp_spin_unlock(&head->lock); +next_nolock: } while (--remaining > 0); otherwise, it will loop forever if all ports were reserved. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 3/3] net: reserve ports for applications using fixed port numbers 2010-04-09 13:21 ` Tetsuo Handa @ 2010-04-12 6:52 ` Cong Wang 2010-04-12 7:06 ` Tetsuo Handa 0 siblings, 1 reply; 24+ messages in thread From: Cong Wang @ 2010-04-12 6:52 UTC (permalink / raw) To: Tetsuo Handa Cc: linux-kernel, opurdila, eric.dumazet, netdev, nhorman, davem, ebiederm Tetsuo Handa wrote: > Hello. > > Amerigo Wang wrote: >> Index: linux-2.6/drivers/infiniband/core/cma.c >> =================================================================== >> --- linux-2.6.orig/drivers/infiniband/core/cma.c >> +++ linux-2.6/drivers/infiniband/core/cma.c >> @@ -1980,6 +1980,8 @@ retry: >> /* FIXME: add proper port randomization per like inet_csk_get_port */ >> do { >> ret = idr_get_new_above(ps, bind_list, next_port, &port); >> + if (inet_is_reserved_local_port(port)) >> + ret = -EAGAIN; > > You should not overwrite ret with -EAGAIN when idr_get_new_above() returned > -ENOSPC. I don't know about idr, thus I don't know whether > > if (!ret && inet_is_reserved_local_port(port)) > ret = -EAGAIN; > > is correct or not. Hmm, good catch! I think it is correct. > >> } while ((ret == -EAGAIN) && idr_pre_get(ps, GFP_KERNEL)); >> >> if (ret) >> @@ -2996,10 +2998,13 @@ static int __init cma_init(void) >> { >> int ret, low, high, remaining; >> >> - get_random_bytes(&next_port, sizeof next_port); >> inet_get_local_port_range(&low, &high); >> +again: >> + get_random_bytes(&next_port, sizeof next_port); >> remaining = (high - low) + 1; >> next_port = ((unsigned int) next_port % remaining) + low; >> + if (inet_is_reserved_local_port(next_port)) >> + goto again; >> > > You should not unconditionally "goto again;". > If all ports were reserved, it will loop forever (CPU stalls). > Yeah, how about: int tries = 10; ... again: ... if (inet_is_reserved_local_port(next_port)) { if (tries--) goto again; else return -EBUSY; } ? >> cma_wq = create_singlethread_workqueue("rdma_cm"); >> if (!cma_wq) > > >> Index: linux-2.6/net/sctp/socket.c >> =================================================================== >> --- linux-2.6.orig/net/sctp/socket.c >> +++ linux-2.6/net/sctp/socket.c >> @@ -5436,6 +5436,8 @@ static long sctp_get_port_local(struct s >> rover++; >> if ((rover < low) || (rover > high)) >> rover = low; >> + if (inet_is_reserved_local_port(rover)) >> + continue; > > This one needs to be > > if (inet_is_reserved_local_port(rover)) > goto next_nolock; > >> index = sctp_phashfn(rover); >> head = &sctp_port_hashtable[index]; >> sctp_spin_lock(&head->lock); > > next: > sctp_spin_unlock(&head->lock); > +next_nolock: > } while (--remaining > 0); > > otherwise, it will loop forever if all ports were reserved. Sorry, doesn't 'continue' jump to exactly where 'next_nolock' is?? Or I am missing something? Thanks for your review! ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 3/3] net: reserve ports for applications using fixed port numbers 2010-04-12 6:52 ` Cong Wang @ 2010-04-12 7:06 ` Tetsuo Handa 0 siblings, 0 replies; 24+ messages in thread From: Tetsuo Handa @ 2010-04-12 7:06 UTC (permalink / raw) To: amwang Cc: linux-kernel, opurdila, eric.dumazet, netdev, nhorman, davem, ebiederm Cong Wang wrote: > Tetsuo Handa wrote: > >> Index: linux-2.6/net/sctp/socket.c > >> =================================================================== > >> --- linux-2.6.orig/net/sctp/socket.c > >> +++ linux-2.6/net/sctp/socket.c > >> @@ -5436,6 +5436,8 @@ static long sctp_get_port_local(struct s > >> rover++; > >> if ((rover < low) || (rover > high)) > >> rover = low; > >> + if (inet_is_reserved_local_port(rover)) > >> + continue; > > > > This one needs to be > > > > if (inet_is_reserved_local_port(rover)) > > goto next_nolock; > > > >> index = sctp_phashfn(rover); > >> head = &sctp_port_hashtable[index]; > >> sctp_spin_lock(&head->lock); > > > > next: > > sctp_spin_unlock(&head->lock); > > +next_nolock: > > } while (--remaining > 0); > > > > otherwise, it will loop forever if all ports were reserved. > > Sorry, doesn't 'continue' jump to exactly where 'next_nolock' is?? > Or I am missing something? My misreading, sorry. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Patch v8 0/3] net: reserve ports for applications using fixed port numbers @ 2010-04-12 10:03 Amerigo Wang 2010-04-12 10:04 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang 0 siblings, 1 reply; 24+ messages in thread From: Amerigo Wang @ 2010-04-12 10:03 UTC (permalink / raw) To: linux-kernel Cc: Octavian Purdila, ebiederm, Eric Dumazet, penguin-kernel, netdev, Neil Horman, Amerigo Wang, David Miller Changes from the previous version: - Rename proc_{get,put}_ulong to proc_{get,put}_long(); - Fix potential dead loop problems in cma code. -------------> This patch introduces /proc/sys/net/ipv4/ip_local_reserved_ports which allows users to reserve ports for third-party applications. The reserved ports will not be used by automatic port assignments (e.g. when calling connect() or bind() with port number 0). Explicit port allocation behavior is unchanged. There are still some miss behaviors with regard to proc parsing in odd invalid cases (for "40000\0-40001" all is acknowledged but only 40000 is accepted) but they are not easy to fix without changing the current "acknowledge how much we accepted" behavior. Because of that and because the same issues are present in the existing proc_dointvec code as well I don't think its worth holding the actual feature (port reservation) after such petty error recovery issues. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Patch 1/3] sysctl: refactor integer handling proc code 2010-04-12 10:03 [Patch v8 0/3] " Amerigo Wang @ 2010-04-12 10:04 ` Amerigo Wang 2010-04-12 10:18 ` Alexey Dobriyan 2010-04-12 10:18 ` Alexey Dobriyan 0 siblings, 2 replies; 24+ messages in thread From: Amerigo Wang @ 2010-04-12 10:04 UTC (permalink / raw) To: linux-kernel Cc: Octavian Purdila, Eric Dumazet, penguin-kernel, netdev, Neil Horman, ebiederm, David Miller, Amerigo Wang From: Octavian Purdila <opurdila@ixiacom.com> As we are about to add another integer handling proc function a little bit of cleanup is in order: add a few helper functions to improve code readability and decrease code duplication. In the process a bug is also fixed: if the user specifies a number with more then 20 digits it will be interpreted as two integers (e.g. 10000...13 will be interpreted as 100.... and 13). Behavior for EFAULT handling was changed as well. Previous to this patch, when an EFAULT error occurred in the middle of a write operation, although some of the elements were set, that was not acknowledged to the user (by shorting the write and returning the number of bytes accepted). EFAULT is now treated just like any other errors by acknowledging the amount of bytes accepted. Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> Signed-off-by: WANG Cong <amwang@redhat.com> Cc: Eric W. Biederman <ebiederm@xmission.com> --- Index: linux-2.6/kernel/sysctl.c =================================================================== --- linux-2.6.orig/kernel/sysctl.c +++ linux-2.6/kernel/sysctl.c @@ -2040,8 +2040,148 @@ int proc_dostring(struct ctl_table *tabl buffer, lenp, ppos); } +static int proc_skip_wspace(char __user **buf, size_t *size) +{ + char c; + + while (*size) { + if (get_user(c, *buf)) + return -EFAULT; + if (!isspace(c)) + break; + (*size)--; + (*buf)++; + } + + return 0; +} + +static bool isanyof(char c, const char *v, unsigned len) +{ + int i; + + if (!len) + return false; + + for (i = 0; i < len; i++) + if (c == v[i]) + break; + if (i == len) + return false; + + return true; +} + +#define TMPBUFLEN 22 +/** + * proc_get_long - reads an ASCII formated integer from a user buffer + * + * @buf - user buffer + * @size - size of the user buffer + * @val - this is where the number will be stored + * @neg - set to %TRUE if number is negative + * @perm_tr - a vector which contains the allowed trailers + * @perm_tr_len - size of the perm_tr vector + * @tr - pointer to store the trailer character + * + * In case of success 0 is returned and buf and size are updated with + * the amount of bytes read. If tr is non NULL and a trailing + * character exist (size is non zero after returning from this + * function) tr is updated with the trailing character. + */ +static int proc_get_long(char __user **buf, size_t *size, + unsigned long *val, bool *neg, + const char *perm_tr, unsigned perm_tr_len, char *tr) +{ + int len; + char *p, tmp[TMPBUFLEN]; + + if (!*size) + return -EINVAL; + + len = *size; + if (len > TMPBUFLEN-1) + len = TMPBUFLEN-1; + + if (copy_from_user(tmp, *buf, len)) + return -EFAULT; + + tmp[len] = 0; + p = tmp; + if (*p == '-' && *size > 1) { + *neg = 1; + p++; + } else + *neg = 0; + if (!isdigit(*p)) + return -EINVAL; + + *val = simple_strtoul(p, &p, 0); + + len = p - tmp; + + /* We don't know if the next char is whitespace thus we may accept + * invalid integers (e.g. 1234...a) or two integers instead of one + * (e.g. 123...1). So lets not allow such large numbers. */ + if (len == TMPBUFLEN - 1) + return -EINVAL; + + if (len < *size && perm_tr_len && !isanyof(*p, perm_tr, perm_tr_len)) + return -EINVAL; + + if (tr && (len < *size)) + *tr = *p; + + *buf += len; + *size -= len; + + return 0; +} + +/** + * proc_put_long - coverts an integer to a decimal ASCII formated string + * + * @buf - the user buffer + * @size - the size of the user buffer + * @val - the integer to be converted + * @neg - sign of the number, %TRUE for negative + * @first - if %FALSE will insert a separator character before the number + * @separator - the separator character + * + * In case of success 0 is returned and buf and size are updated with + * the amount of bytes read. + */ +static int proc_put_long(char __user **buf, size_t *size, unsigned long val, + bool neg, bool first, char separator) +{ + int len; + char tmp[TMPBUFLEN], *p = tmp; + + if (!first) + *p++ = separator; + sprintf(p, "%s%lu", neg ? "-" : "", val); + len = strlen(tmp); + if (len > *size) + len = *size; + if (copy_to_user(*buf, tmp, len)) + return -EFAULT; + *size -= len; + *buf += len; + return 0; +} +#undef TMPBUFLEN + +static int proc_put_char(char __user **buf, size_t *size, char c) +{ + if (*size) { + if (put_user(c, *buf)) + return -EFAULT; + (*size)--, (*buf)++; + } + return 0; +} -static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2050,7 +2190,7 @@ static int do_proc_dointvec_conv(int *ne } else { int val = *valp; if (val < 0) { - *negp = -1; + *negp = 1; *lvalp = (unsigned long)-val; } else { *negp = 0; @@ -2060,20 +2200,18 @@ static int do_proc_dointvec_conv(int *ne return 0; } +static const char proc_wspace_sep[] = { ' ', '\t', '\n', 0 }; + static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table, - int write, void __user *buffer, + int write, void __user *_buffer, size_t *lenp, loff_t *ppos, - int (*conv)(int *negp, unsigned long *lvalp, int *valp, + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, int write, void *data), void *data) { -#define TMPBUFLEN 21 - int *i, vleft, first = 1, neg; - unsigned long lval; - size_t left, len; - - char buf[TMPBUFLEN], *p; - char __user *s = buffer; + int *i, vleft, first = 1, err = 0; + size_t left; + char __user *buffer = (char __user *) _buffer; if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) { @@ -2089,88 +2227,48 @@ static int __do_proc_dointvec(void *tbl_ conv = do_proc_dointvec_conv; for (; left && vleft--; i++, first=0) { - if (write) { - while (left) { - char c; - if (get_user(c, s)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - s++; - } - if (!left) - break; - neg = 0; - len = left; - if (len > sizeof(buf) - 1) - len = sizeof(buf) - 1; - if (copy_from_user(buf, s, len)) - return -EFAULT; - buf[len] = 0; - p = buf; - if (*p == '-' && left > 1) { - neg = 1; - p++; - } - if (*p < '0' || *p > '9') - break; - - lval = simple_strtoul(p, &p, 0); + unsigned long lval; + bool neg; - len = p-buf; - if ((len < left) && *p && !isspace(*p)) + if (write) { + err = proc_skip_wspace(&buffer, &left); + if (err) + return err; + err = proc_get_long(&buffer, &left, &lval, &neg, + proc_wspace_sep, + sizeof(proc_wspace_sep), NULL); + if (err) break; - s += len; - left -= len; - - if (conv(&neg, &lval, i, 1, data)) + if (conv(&neg, &lval, i, 1, data)) { + err = -EINVAL; break; + } } else { - p = buf; - if (!first) - *p++ = '\t'; - - if (conv(&neg, &lval, i, 0, data)) + if (conv(&neg, &lval, i, 0, data)) { + err = -EINVAL; break; - - sprintf(p, "%s%lu", neg ? "-" : "", lval); - len = strlen(buf); - if (len > left) - len = left; - if(copy_to_user(s, buf, len)) - return -EFAULT; - left -= len; - s += len; - } - } - - if (!write && !first && left) { - if(put_user('\n', s)) - return -EFAULT; - left--, s++; - } - if (write) { - while (left) { - char c; - if (get_user(c, s++)) - return -EFAULT; - if (!isspace(c)) + } + err = proc_put_long(&buffer, &left, lval, neg, first, + '\t'); + if (err) break; - left--; } } + + if (!write && !first && left && !err) + err = proc_put_char(&buffer, &left, '\n'); + if (write && !err) + err = proc_skip_wspace(&buffer, &left); if (write && first) - return -EINVAL; + return err ? : -EINVAL; *lenp -= left; *ppos += *lenp; return 0; -#undef TMPBUFLEN } static int do_proc_dointvec(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos, - int (*conv)(int *negp, unsigned long *lvalp, int *valp, + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, int write, void *data), void *data) { @@ -2238,8 +2336,8 @@ struct do_proc_dointvec_minmax_conv_para int *max; }; -static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp, - int *valp, +static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp, + int *valp, int write, void *data) { struct do_proc_dointvec_minmax_conv_param *param = data; @@ -2252,7 +2350,7 @@ static int do_proc_dointvec_minmax_conv( } else { int val = *valp; if (val < 0) { - *negp = -1; + *negp = 1; *lvalp = (unsigned long)-val; } else { *negp = 0; @@ -2290,17 +2388,15 @@ int proc_dointvec_minmax(struct ctl_tabl } static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write, - void __user *buffer, + void __user *_buffer, size_t *lenp, loff_t *ppos, unsigned long convmul, unsigned long convdiv) { -#define TMPBUFLEN 21 - unsigned long *i, *min, *max, val; - int vleft, first=1, neg; - size_t len, left; - char buf[TMPBUFLEN], *p; - char __user *s = buffer; + unsigned long *i, *min, *max; + int vleft, first = 1, err = 0; + size_t left; + char __user *buffer = (char __user *) _buffer; if (!data || !table->maxlen || !*lenp || (*ppos && !write)) { @@ -2315,82 +2411,42 @@ static int __do_proc_doulongvec_minmax(v left = *lenp; for (; left && vleft--; i++, min++, max++, first=0) { + unsigned long val; + if (write) { - while (left) { - char c; - if (get_user(c, s)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - s++; - } - if (!left) - break; - neg = 0; - len = left; - if (len > TMPBUFLEN-1) - len = TMPBUFLEN-1; - if (copy_from_user(buf, s, len)) - return -EFAULT; - buf[len] = 0; - p = buf; - if (*p == '-' && left > 1) { - neg = 1; - p++; - } - if (*p < '0' || *p > '9') - break; - val = simple_strtoul(p, &p, 0) * convmul / convdiv ; - len = p-buf; - if ((len < left) && *p && !isspace(*p)) + bool neg; + + err = proc_skip_wspace(&buffer, &left); + if (err) + return err; + err = proc_get_long(&buffer, &left, &val, &neg, + proc_wspace_sep, + sizeof(proc_wspace_sep), NULL); + if (err) break; if (neg) - val = -val; - s += len; - left -= len; - - if(neg) continue; if ((min && val < *min) || (max && val > *max)) continue; *i = val; } else { - p = buf; - if (!first) - *p++ = '\t'; - sprintf(p, "%lu", convdiv * (*i) / convmul); - len = strlen(buf); - if (len > left) - len = left; - if(copy_to_user(s, buf, len)) - return -EFAULT; - left -= len; - s += len; - } - } - - if (!write && !first && left) { - if(put_user('\n', s)) - return -EFAULT; - left--, s++; - } - if (write) { - while (left) { - char c; - if (get_user(c, s++)) - return -EFAULT; - if (!isspace(c)) + val = convdiv * (*i) / convmul; + err = proc_put_long(&buffer, &left, val, 0, first, + '\t'); + if (err) break; - left--; } } + + if (!write && !first && left && !err) + err = proc_put_char(&buffer, &left, '\n'); + if (write && !err) + err = proc_skip_wspace(&buffer, &left); if (write && first) - return -EINVAL; + return err ? : -EINVAL; *lenp -= left; *ppos += *lenp; return 0; -#undef TMPBUFLEN } static int do_proc_doulongvec_minmax(struct ctl_table *table, int write, @@ -2451,7 +2507,7 @@ int proc_doulongvec_ms_jiffies_minmax(st } -static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2463,7 +2519,7 @@ static int do_proc_dointvec_jiffies_conv int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0; @@ -2474,7 +2530,7 @@ static int do_proc_dointvec_jiffies_conv return 0; } -static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_userhz_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2486,7 +2542,7 @@ static int do_proc_dointvec_userhz_jiffi int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0; @@ -2497,7 +2553,7 @@ static int do_proc_dointvec_userhz_jiffi return 0; } -static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_ms_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2507,7 +2563,7 @@ static int do_proc_dointvec_ms_jiffies_c int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0; ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 1/3] sysctl: refactor integer handling proc code 2010-04-12 10:04 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang @ 2010-04-12 10:18 ` Alexey Dobriyan 2010-04-12 10:18 ` Alexey Dobriyan 1 sibling, 0 replies; 24+ messages in thread From: Alexey Dobriyan @ 2010-04-12 10:18 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, Octavian Purdila, Eric Dumazet, penguin-kernel, netdev, Neil Horman, ebiederm, David Miller On Mon, Apr 12, 2010 at 06:04:04AM -0400, Amerigo Wang wrote: > As we are about to add another integer handling proc function a little > bit of cleanup is in order: add a few helper functions to improve code > readability and decrease code duplication. > > In the process a bug is also fixed: if the user specifies a number > with more then 20 digits it will be interpreted as two integers > (e.g. 10000...13 will be interpreted as 100.... and 13). ULONG_MAX is not 22 digits always. The fix is to not rely on simple_strtoul() I guess it's time to finally remove it. :-( Also, it's better to copy_from user stuff once. Without looking at non-trivial users, one page should be enough. > Behavior for EFAULT handling was changed as well. Previous to this > patch, when an EFAULT error occurred in the middle of a write > operation, although some of the elements were set, that was not > acknowledged to the user (by shorting the write and returning the > number of bytes accepted). EFAULT is now treated just like any other > errors by acknowledging the amount of bytes accepted. > +static int proc_skip_wspace(char __user **buf, size_t *size) > +{ > + char c; > + > + while (*size) { > + if (get_user(c, *buf)) > + return -EFAULT; > + if (!isspace(c)) > + break; > + (*size)--; > + (*buf)++; > + } > + > + return 0; > +} yeah, copy_from_user once, so we won't have this. > +static bool isanyof(char c, const char *v, unsigned len) A what? this is memchr() > +{ > + int i; > + > + if (!len) > + return false; > + > + for (i = 0; i < len; i++) > + if (c == v[i]) > + break; > + if (i == len) > + return false; > + > + return true; > +} > + > +#define TMPBUFLEN 22 > +/** > + * proc_get_long - reads an ASCII formated integer from a user buffer > + * > + * @buf - user buffer > + * @size - size of the user buffer > + * @val - this is where the number will be stored > + * @neg - set to %TRUE if number is negative > + * @perm_tr - a vector which contains the allowed trailers > + * @perm_tr_len - size of the perm_tr vector > + * @tr - pointer to store the trailer character > + * > + * In case of success 0 is returned and buf and size are updated with > + * the amount of bytes read. If tr is non NULL and a trailing > + * character exist (size is non zero after returning from this > + * function) tr is updated with the trailing character. > + */ > +static int proc_get_long(char __user **buf, size_t *size, > + unsigned long *val, bool *neg, > + const char *perm_tr, unsigned perm_tr_len, char *tr) > +{ > + int len; > + char *p, tmp[TMPBUFLEN]; > + > + if (!*size) > + return -EINVAL; > + > + len = *size; > + if (len > TMPBUFLEN-1) > + len = TMPBUFLEN-1; > + > + if (copy_from_user(tmp, *buf, len)) > + return -EFAULT; > + > + tmp[len] = 0; > + p = tmp; > + if (*p == '-' && *size > 1) { > + *neg = 1; > + p++; > + } else > + *neg = 0; > + if (!isdigit(*p)) > + return -EINVAL; > + > + *val = simple_strtoul(p, &p, 0); > + > + len = p - tmp; > + > + /* We don't know if the next char is whitespace thus we may accept > + * invalid integers (e.g. 1234...a) or two integers instead of one > + * (e.g. 123...1). So lets not allow such large numbers. */ > + if (len == TMPBUFLEN - 1) > + return -EINVAL; > + > + if (len < *size && perm_tr_len && !isanyof(*p, perm_tr, perm_tr_len)) > + return -EINVAL; > + > + if (tr && (len < *size)) > + *tr = *p; > + > + *buf += len; > + *size -= len; > + > + return 0; > +} ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 1/3] sysctl: refactor integer handling proc code 2010-04-12 10:04 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang @ 2010-04-12 10:18 ` Alexey Dobriyan 2010-04-12 10:18 ` Alexey Dobriyan 1 sibling, 0 replies; 24+ messages in thread From: Alexey Dobriyan @ 2010-04-13 11:18 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, Octavian Purdila, Eric Dumazet, penguin-kernel, netdev, Neil Horman, ebiederm, David Miller On Mon, Apr 12, 2010 at 06:04:04AM -0400, Amerigo Wang wrote: > As we are about to add another integer handling proc function a little > bit of cleanup is in order: add a few helper functions to improve code > readability and decrease code duplication. > > In the process a bug is also fixed: if the user specifies a number > with more then 20 digits it will be interpreted as two integers > (e.g. 10000...13 will be interpreted as 100.... and 13). ULONG_MAX is not 22 digits always. The fix is to not rely on simple_strtoul() I guess it's time to finally remove it. :-( Also, it's better to copy_from user stuff once. Without looking at non-trivial users, one page should be enough. > Behavior for EFAULT handling was changed as well. Previous to this > patch, when an EFAULT error occurred in the middle of a write > operation, although some of the elements were set, that was not > acknowledged to the user (by shorting the write and returning the > number of bytes accepted). EFAULT is now treated just like any other > errors by acknowledging the amount of bytes accepted. > +static int proc_skip_wspace(char __user **buf, size_t *size) > +{ > + char c; > + > + while (*size) { > + if (get_user(c, *buf)) > + return -EFAULT; > + if (!isspace(c)) > + break; > + (*size)--; > + (*buf)++; > + } > + > + return 0; > +} yeah, copy_from_user once, so we won't have this. > +static bool isanyof(char c, const char *v, unsigned len) A what? this is memchr() > +{ > + int i; > + > + if (!len) > + return false; > + > + for (i = 0; i < len; i++) > + if (c == v[i]) > + break; > + if (i == len) > + return false; > + > + return true; > +} > + > +#define TMPBUFLEN 22 > +/** > + * proc_get_long - reads an ASCII formated integer from a user buffer > + * > + * @buf - user buffer > + * @size - size of the user buffer > + * @val - this is where the number will be stored > + * @neg - set to %TRUE if number is negative > + * @perm_tr - a vector which contains the allowed trailers > + * @perm_tr_len - size of the perm_tr vector > + * @tr - pointer to store the trailer character > + * > + * In case of success 0 is returned and buf and size are updated with > + * the amount of bytes read. If tr is non NULL and a trailing > + * character exist (size is non zero after returning from this > + * function) tr is updated with the trailing character. > + */ > +static int proc_get_long(char __user **buf, size_t *size, > + unsigned long *val, bool *neg, > + const char *perm_tr, unsigned perm_tr_len, char *tr) > +{ > + int len; > + char *p, tmp[TMPBUFLEN]; > + > + if (!*size) > + return -EINVAL; > + > + len = *size; > + if (len > TMPBUFLEN-1) > + len = TMPBUFLEN-1; > + > + if (copy_from_user(tmp, *buf, len)) > + return -EFAULT; > + > + tmp[len] = 0; > + p = tmp; > + if (*p == '-' && *size > 1) { > + *neg = 1; > + p++; > + } else > + *neg = 0; > + if (!isdigit(*p)) > + return -EINVAL; > + > + *val = simple_strtoul(p, &p, 0); > + > + len = p - tmp; > + > + /* We don't know if the next char is whitespace thus we may accept > + * invalid integers (e.g. 1234...a) or two integers instead of one > + * (e.g. 123...1). So lets not allow such large numbers. */ > + if (len == TMPBUFLEN - 1) > + return -EINVAL; > + > + if (len < *size && perm_tr_len && !isanyof(*p, perm_tr, perm_tr_len)) > + return -EINVAL; > + > + if (tr && (len < *size)) > + *tr = *p; > + > + *buf += len; > + *size -= len; > + > + return 0; > +} ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 1/3] sysctl: refactor integer handling proc code @ 2010-04-12 10:18 ` Alexey Dobriyan 0 siblings, 0 replies; 24+ messages in thread From: Alexey Dobriyan @ 2010-04-12 10:18 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, Octavian Purdila, Eric Dumazet, penguin-kernel, netdev, Neil Horman, ebiederm, David Miller On Mon, Apr 12, 2010 at 06:04:04AM -0400, Amerigo Wang wrote: > As we are about to add another integer handling proc function a little > bit of cleanup is in order: add a few helper functions to improve code > readability and decrease code duplication. > > In the process a bug is also fixed: if the user specifies a number > with more then 20 digits it will be interpreted as two integers > (e.g. 10000...13 will be interpreted as 100.... and 13). ULONG_MAX is not 22 digits always. The fix is to not rely on simple_strtoul() I guess it's time to finally remove it. :-( Also, it's better to copy_from user stuff once. Without looking at non-trivial users, one page should be enough. > Behavior for EFAULT handling was changed as well. Previous to this > patch, when an EFAULT error occurred in the middle of a write > operation, although some of the elements were set, that was not > acknowledged to the user (by shorting the write and returning the > number of bytes accepted). EFAULT is now treated just like any other > errors by acknowledging the amount of bytes accepted. > +static int proc_skip_wspace(char __user **buf, size_t *size) > +{ > + char c; > + > + while (*size) { > + if (get_user(c, *buf)) > + return -EFAULT; > + if (!isspace(c)) > + break; > + (*size)--; > + (*buf)++; > + } > + > + return 0; > +} yeah, copy_from_user once, so we won't have this. > +static bool isanyof(char c, const char *v, unsigned len) A what? this is memchr() > +{ > + int i; > + > + if (!len) > + return false; > + > + for (i = 0; i < len; i++) > + if (c == v[i]) > + break; > + if (i == len) > + return false; > + > + return true; > +} > + > +#define TMPBUFLEN 22 > +/** > + * proc_get_long - reads an ASCII formated integer from a user buffer > + * > + * @buf - user buffer > + * @size - size of the user buffer > + * @val - this is where the number will be stored > + * @neg - set to %TRUE if number is negative > + * @perm_tr - a vector which contains the allowed trailers > + * @perm_tr_len - size of the perm_tr vector > + * @tr - pointer to store the trailer character > + * > + * In case of success 0 is returned and buf and size are updated with > + * the amount of bytes read. If tr is non NULL and a trailing > + * character exist (size is non zero after returning from this > + * function) tr is updated with the trailing character. > + */ > +static int proc_get_long(char __user **buf, size_t *size, > + unsigned long *val, bool *neg, > + const char *perm_tr, unsigned perm_tr_len, char *tr) > +{ > + int len; > + char *p, tmp[TMPBUFLEN]; > + > + if (!*size) > + return -EINVAL; > + > + len = *size; > + if (len > TMPBUFLEN-1) > + len = TMPBUFLEN-1; > + > + if (copy_from_user(tmp, *buf, len)) > + return -EFAULT; > + > + tmp[len] = 0; > + p = tmp; > + if (*p == '-' && *size > 1) { > + *neg = 1; > + p++; > + } else > + *neg = 0; > + if (!isdigit(*p)) > + return -EINVAL; > + > + *val = simple_strtoul(p, &p, 0); > + > + len = p - tmp; > + > + /* We don't know if the next char is whitespace thus we may accept > + * invalid integers (e.g. 1234...a) or two integers instead of one > + * (e.g. 123...1). So lets not allow such large numbers. */ > + if (len == TMPBUFLEN - 1) > + return -EINVAL; > + > + if (len < *size && perm_tr_len && !isanyof(*p, perm_tr, perm_tr_len)) > + return -EINVAL; > + > + if (tr && (len < *size)) > + *tr = *p; > + > + *buf += len; > + *size -= len; > + > + return 0; > +} ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 1/3] sysctl: refactor integer handling proc code 2010-04-12 10:18 ` Alexey Dobriyan (?) @ 2010-04-13 7:35 ` Cong Wang -1 siblings, 0 replies; 24+ messages in thread From: Cong Wang @ 2010-04-13 7:35 UTC (permalink / raw) To: Alexey Dobriyan Cc: linux-kernel, Octavian Purdila, Eric Dumazet, penguin-kernel, netdev, Neil Horman, ebiederm, David Miller Alexey Dobriyan wrote: > On Mon, Apr 12, 2010 at 06:04:04AM -0400, Amerigo Wang wrote: >> As we are about to add another integer handling proc function a little >> bit of cleanup is in order: add a few helper functions to improve code >> readability and decrease code duplication. >> >> In the process a bug is also fixed: if the user specifies a number >> with more then 20 digits it will be interpreted as two integers >> (e.g. 10000...13 will be interpreted as 100.... and 13). > > ULONG_MAX is not 22 digits always. > > The fix is to not rely on simple_strtoul() > > I guess it's time to finally remove it. :-( Or use strict_strtoul()? > > Also, it's better to copy_from user stuff once. > Without looking at non-trivial users, one page should be enough. It seems that all proc code assumes that the input buffer will not exceed one page size. > >> Behavior for EFAULT handling was changed as well. Previous to this >> patch, when an EFAULT error occurred in the middle of a write >> operation, although some of the elements were set, that was not >> acknowledged to the user (by shorting the write and returning the >> number of bytes accepted). EFAULT is now treated just like any other >> errors by acknowledging the amount of bytes accepted. > >> +static int proc_skip_wspace(char __user **buf, size_t *size) >> +{ >> + char c; >> + >> + while (*size) { >> + if (get_user(c, *buf)) >> + return -EFAULT; >> + if (!isspace(c)) >> + break; >> + (*size)--; >> + (*buf)++; >> + } >> + >> + return 0; >> +} > > yeah, copy_from_user once, so we won't have this. Ok. > >> +static bool isanyof(char c, const char *v, unsigned len) > > A what? > this is memchr() > Hmm, right, it should be memchr(v, c, len). Thanks! ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Patch v9 0/3] net: reserve ports for applications using fixed port numbers @ 2010-04-30 8:25 Amerigo Wang 2010-04-30 8:25 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang 0 siblings, 1 reply; 24+ messages in thread From: Amerigo Wang @ 2010-04-30 8:25 UTC (permalink / raw) To: linux-kernel Cc: Octavian Purdila, ebiederm, Eric Dumazet, penguin-kernel, netdev, Neil Horman, Amerigo Wang, adobriyan, David Miller Changes from the previous version: - Dropped the infiniband part, because Tetsuo modified the related code, I will send a separate patch for it once this is accepted. - Fixed some '\0' issues introduced by the previous version. - Use copy_from_user(), instead of get_user(). - Use memchr(). ------------------> This patch introduces /proc/sys/net/ipv4/ip_local_reserved_ports which allows users to reserve ports for third-party applications. The reserved ports will not be used by automatic port assignments (e.g. when calling connect() or bind() with port number 0). Explicit port allocation behavior is unchanged. There are still some miss behaviors with regard to proc parsing in odd invalid cases (for "40000\0-40001" all is acknowledged but only 40000 is accepted) but they are not easy to fix without changing the current "acknowledge how much we accepted" behavior. Because of that and because the same issues are present in the existing proc_dointvec code as well I don't think its worth holding the actual feature (port reservation) after such petty error recovery issues. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Patch 1/3] sysctl: refactor integer handling proc code 2010-04-30 8:25 [Patch v9 0/3] net: reserve ports for applications using fixed port numbers Amerigo Wang @ 2010-04-30 8:25 ` Amerigo Wang 2010-04-30 22:49 ` Changli Gao 0 siblings, 1 reply; 24+ messages in thread From: Amerigo Wang @ 2010-04-30 8:25 UTC (permalink / raw) To: linux-kernel Cc: Octavian Purdila, Eric Dumazet, penguin-kernel, netdev, Neil Horman, ebiederm, David Miller, adobriyan, Amerigo Wang (Based on Octavian's work, and I modified a lot.) As we are about to add another integer handling proc function a little bit of cleanup is in order: add a few helper functions to improve code readability and decrease code duplication. In the process a bug is also fixed: if the user specifies a number with more then 20 digits it will be interpreted as two integers (e.g. 10000...13 will be interpreted as 100.... and 13). Behavior for EFAULT handling was changed as well. Previous to this patch, when an EFAULT error occurred in the middle of a write operation, although some of the elements were set, that was not acknowledged to the user (by shorting the write and returning the number of bytes accepted). EFAULT is now treated just like any other errors by acknowledging the amount of bytes accepted. Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> Signed-off-by: WANG Cong <amwang@redhat.com> Cc: Eric W. Biederman <ebiederm@xmission.com> --- Index: linux-2.6/kernel/sysctl.c =================================================================== --- linux-2.6.orig/kernel/sysctl.c +++ linux-2.6/kernel/sysctl.c @@ -2040,8 +2040,122 @@ int proc_dostring(struct ctl_table *tabl buffer, lenp, ppos); } +static size_t proc_skip_spaces(char **buf) +{ + size_t ret; + char *tmp = skip_spaces(*buf); + ret = tmp - *buf; + *buf = tmp; + return ret; +} + +#define TMPBUFLEN 22 +/** + * proc_get_long - reads an ASCII formated integer from a user buffer + * + * @buf - a kernel buffer + * @size - size of the kernel buffer + * @val - this is where the number will be stored + * @neg - set to %TRUE if number is negative + * @perm_tr - a vector which contains the allowed trailers + * @perm_tr_len - size of the perm_tr vector + * @tr - pointer to store the trailer character + * + * In case of success 0 is returned and buf and size are updated with + * the amount of bytes read. If tr is non NULL and a trailing + * character exist (size is non zero after returning from this + * function) tr is updated with the trailing character. + */ +static int proc_get_long(char **buf, size_t *size, + unsigned long *val, bool *neg, + const char *perm_tr, unsigned perm_tr_len, char *tr) +{ + int len; + char *p, tmp[TMPBUFLEN]; + + if (!*size) + return -EINVAL; + + len = *size; + if (len > TMPBUFLEN-1) + len = TMPBUFLEN-1; + + memcpy(tmp, *buf, len); + + tmp[len] = 0; + p = tmp; + if (*p == '-' && *size > 1) { + *neg = 1; + p++; + } else + *neg = 0; + if (!isdigit(*p)) + return -EINVAL; + + *val = simple_strtoul(p, &p, 0); -static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp, + len = p - tmp; + + /* We don't know if the next char is whitespace thus we may accept + * invalid integers (e.g. 1234...a) or two integers instead of one + * (e.g. 123...1). So lets not allow such large numbers. */ + if (len == TMPBUFLEN - 1) + return -EINVAL; + + if (len < *size && perm_tr_len && !memchr(perm_tr, *p, perm_tr_len)) + return -EINVAL; + + if (tr && (len < *size)) + *tr = *p; + + *buf += len; + *size -= len; + + return 0; +} + +/** + * proc_put_long - coverts an integer to a decimal ASCII formated string + * + * @buf - the user buffer + * @size - the size of the user buffer + * @val - the integer to be converted + * @neg - sign of the number, %TRUE for negative + * + * In case of success 0 is returned and buf and size are updated with + * the amount of bytes read. + */ +static int proc_put_long(void __user **buf, size_t *size, unsigned long val, + bool neg) +{ + int len; + char tmp[TMPBUFLEN], *p = tmp; + + sprintf(p, "%s%lu", neg ? "-" : "", val); + len = strlen(tmp); + if (len > *size) + len = *size; + if (copy_to_user(*buf, tmp, len)) + return -EFAULT; + *size -= len; + *buf += len; + return 0; +} +#undef TMPBUFLEN + +static int proc_put_char(void __user **buf, size_t *size, char c) +{ + if (*size) { + char __user **buffer = (char __user **)buf; + if (put_user(c, *buffer)) + return -EFAULT; + (*size)--, (*buffer)++; + *buf = *buffer; + } + return 0; +} + +static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2050,7 +2164,7 @@ static int do_proc_dointvec_conv(int *ne } else { int val = *valp; if (val < 0) { - *negp = -1; + *negp = 1; *lvalp = (unsigned long)-val; } else { *negp = 0; @@ -2060,23 +2174,21 @@ static int do_proc_dointvec_conv(int *ne return 0; } +static const char proc_wspace_sep[] = { ' ', '\t', '\n' }; + static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos, - int (*conv)(int *negp, unsigned long *lvalp, int *valp, + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, int write, void *data), void *data) { -#define TMPBUFLEN 21 - int *i, vleft, first = 1, neg; - unsigned long lval; - size_t left, len; + int *i, vleft, first = 1, err = 0; + unsigned long page = 0; + size_t left; + char *kbuf; - char buf[TMPBUFLEN], *p; - char __user *s = buffer; - - if (!tbl_data || !table->maxlen || !*lenp || - (*ppos && !write)) { + if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) { *lenp = 0; return 0; } @@ -2088,89 +2200,69 @@ static int __do_proc_dointvec(void *tbl_ if (!conv) conv = do_proc_dointvec_conv; + if (write) { + if (left > PAGE_SIZE - 1) + left = PAGE_SIZE - 1; + page = __get_free_page(GFP_TEMPORARY); + kbuf = (char *) page; + if (!kbuf) + return -ENOMEM; + if (copy_from_user(kbuf, buffer, left)) { + err = -EFAULT; + goto free; + } + kbuf[left] = 0; + } + for (; left && vleft--; i++, first=0) { - if (write) { - while (left) { - char c; - if (get_user(c, s)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - s++; - } - if (!left) - break; - neg = 0; - len = left; - if (len > sizeof(buf) - 1) - len = sizeof(buf) - 1; - if (copy_from_user(buf, s, len)) - return -EFAULT; - buf[len] = 0; - p = buf; - if (*p == '-' && left > 1) { - neg = 1; - p++; - } - if (*p < '0' || *p > '9') - break; + unsigned long lval; + bool neg; - lval = simple_strtoul(p, &p, 0); + if (write) { + left -= proc_skip_spaces(&kbuf); - len = p-buf; - if ((len < left) && *p && !isspace(*p)) + err = proc_get_long(&kbuf, &left, &lval, &neg, + proc_wspace_sep, + sizeof(proc_wspace_sep), NULL); + if (err) break; - s += len; - left -= len; - - if (conv(&neg, &lval, i, 1, data)) + if (conv(&neg, &lval, i, 1, data)) { + err = -EINVAL; break; + } } else { - p = buf; + if (conv(&neg, &lval, i, 0, data)) { + err = -EINVAL; + break; + } if (!first) - *p++ = '\t'; - - if (conv(&neg, &lval, i, 0, data)) + err = proc_put_char(&buffer, &left, '\t'); + if (err) + break; + err = proc_put_long(&buffer, &left, lval, neg); + if (err) break; - - sprintf(p, "%s%lu", neg ? "-" : "", lval); - len = strlen(buf); - if (len > left) - len = left; - if(copy_to_user(s, buf, len)) - return -EFAULT; - left -= len; - s += len; } } - if (!write && !first && left) { - if(put_user('\n', s)) - return -EFAULT; - left--, s++; - } + if (!write && !first && left && !err) + err = proc_put_char(&buffer, &left, '\n'); + if (write && !err) + left -= proc_skip_spaces(&kbuf); +free: if (write) { - while (left) { - char c; - if (get_user(c, s++)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - } + free_page(page); + if (first) + return err ? : -EINVAL; } - if (write && first) - return -EINVAL; *lenp -= left; *ppos += *lenp; - return 0; -#undef TMPBUFLEN + return err; } static int do_proc_dointvec(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos, - int (*conv)(int *negp, unsigned long *lvalp, int *valp, + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, int write, void *data), void *data) { @@ -2238,8 +2330,8 @@ struct do_proc_dointvec_minmax_conv_para int *max; }; -static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp, - int *valp, +static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp, + int *valp, int write, void *data) { struct do_proc_dointvec_minmax_conv_param *param = data; @@ -2252,7 +2344,7 @@ static int do_proc_dointvec_minmax_conv( } else { int val = *valp; if (val < 0) { - *negp = -1; + *negp = 1; *lvalp = (unsigned long)-val; } else { *negp = 0; @@ -2295,102 +2387,78 @@ static int __do_proc_doulongvec_minmax(v unsigned long convmul, unsigned long convdiv) { -#define TMPBUFLEN 21 - unsigned long *i, *min, *max, val; - int vleft, first=1, neg; - size_t len, left; - char buf[TMPBUFLEN], *p; - char __user *s = buffer; - - if (!data || !table->maxlen || !*lenp || - (*ppos && !write)) { + unsigned long *i, *min, *max; + int vleft, first = 1, err = 0; + unsigned long page = 0; + size_t left; + char *kbuf; + + if (!data || !table->maxlen || !*lenp || (*ppos && !write)) { *lenp = 0; return 0; } - + i = (unsigned long *) data; min = (unsigned long *) table->extra1; max = (unsigned long *) table->extra2; vleft = table->maxlen / sizeof(unsigned long); left = *lenp; - + + if (write) { + if (left > PAGE_SIZE - 1) + left = PAGE_SIZE - 1; + page = __get_free_page(GFP_TEMPORARY); + kbuf = (char *) page; + if (!kbuf) + return -ENOMEM; + if (copy_from_user(kbuf, buffer, left)) { + err = -EFAULT; + goto free; + } + kbuf[left] = 0; + } + for (; left && vleft--; i++, min++, max++, first=0) { + unsigned long val; + if (write) { - while (left) { - char c; - if (get_user(c, s)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - s++; - } - if (!left) - break; - neg = 0; - len = left; - if (len > TMPBUFLEN-1) - len = TMPBUFLEN-1; - if (copy_from_user(buf, s, len)) - return -EFAULT; - buf[len] = 0; - p = buf; - if (*p == '-' && left > 1) { - neg = 1; - p++; - } - if (*p < '0' || *p > '9') - break; - val = simple_strtoul(p, &p, 0) * convmul / convdiv ; - len = p-buf; - if ((len < left) && *p && !isspace(*p)) + bool neg; + + left -= proc_skip_spaces(&kbuf); + + err = proc_get_long(&kbuf, &left, &val, &neg, + proc_wspace_sep, + sizeof(proc_wspace_sep), NULL); + if (err) break; if (neg) - val = -val; - s += len; - left -= len; - - if(neg) continue; if ((min && val < *min) || (max && val > *max)) continue; *i = val; } else { - p = buf; + val = convdiv * (*i) / convmul; if (!first) - *p++ = '\t'; - sprintf(p, "%lu", convdiv * (*i) / convmul); - len = strlen(buf); - if (len > left) - len = left; - if(copy_to_user(s, buf, len)) - return -EFAULT; - left -= len; - s += len; + err = proc_put_char(&buffer, &left, '\t'); + err = proc_put_long(&buffer, &left, val, false); + if (err) + break; } } - if (!write && !first && left) { - if(put_user('\n', s)) - return -EFAULT; - left--, s++; - } + if (!write && !first && left && !err) + err = proc_put_char(&buffer, &left, '\n'); + if (write && !err) + left -= proc_skip_spaces(&kbuf); +free: if (write) { - while (left) { - char c; - if (get_user(c, s++)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - } + free_page(page); + if (first) + return err ? : -EINVAL; } - if (write && first) - return -EINVAL; *lenp -= left; *ppos += *lenp; - return 0; -#undef TMPBUFLEN + return err; } static int do_proc_doulongvec_minmax(struct ctl_table *table, int write, @@ -2451,7 +2519,7 @@ int proc_doulongvec_ms_jiffies_minmax(st } -static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2463,7 +2531,7 @@ static int do_proc_dointvec_jiffies_conv int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0; @@ -2474,7 +2542,7 @@ static int do_proc_dointvec_jiffies_conv return 0; } -static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_userhz_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2486,7 +2554,7 @@ static int do_proc_dointvec_userhz_jiffi int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0; @@ -2497,7 +2565,7 @@ static int do_proc_dointvec_userhz_jiffi return 0; } -static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_ms_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2507,7 +2575,7 @@ static int do_proc_dointvec_ms_jiffies_c int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0; ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 1/3] sysctl: refactor integer handling proc code 2010-04-30 8:25 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang @ 2010-04-30 22:49 ` Changli Gao 0 siblings, 0 replies; 24+ messages in thread From: Changli Gao @ 2010-04-30 22:49 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, Octavian Purdila, Eric Dumazet, penguin-kernel, netdev, Neil Horman, ebiederm, David Miller, adobriyan [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 23894 bytes --] On Fri, Apr 30, 2010 at 4:25 PM, Amerigo Wang <amwang@redhat.com> wrote:> (Based on Octavian's work, and I modified a lot.)>> As we are about to add another integer handling proc function a little> bit of cleanup is in order: add a few helper functions to improve code> readability and decrease code duplication.>> In the process a bug is also fixed: if the user specifies a number> with more then 20 digits it will be interpreted as two integers> (e.g. 10000...13 will be interpreted as 100.... and 13).>> Behavior for EFAULT handling was changed as well. Previous to this> patch, when an EFAULT error occurred in the middle of a write> operation, although some of the elements were set, that was not> acknowledged to the user (by shorting the write and returning the> number of bytes accepted). EFAULT is now treated just like any other> errors by acknowledging the amount of bytes accepted.>> Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>> Signed-off-by: WANG Cong <amwang@redhat.com>> Cc: Eric W. Biederman <ebiederm@xmission.com>> --->> Index: linux-2.6/kernel/sysctl.c> ===================================================================> --- linux-2.6.orig/kernel/sysctl.c> +++ linux-2.6/kernel/sysctl.c> @@ -2040,8 +2040,122 @@ int proc_dostring(struct ctl_table *tabl>                buffer, lenp, ppos);>  }>> +static size_t proc_skip_spaces(char **buf)> +{> +    size_t ret;> +    char *tmp = skip_spaces(*buf);> +    ret = tmp - *buf;> +    *buf = tmp;> +    return ret;> +}> +> +#define TMPBUFLEN 22> +/**> + * proc_get_long - reads an ASCII formated integer from a user buffer> + *> + * @buf - a kernel buffer> + * @size - size of the kernel buffer> + * @val - this is where the number will be stored> + * @neg - set to %TRUE if number is negative> + * @perm_tr - a vector which contains the allowed trailers> + * @perm_tr_len - size of the perm_tr vector> + * @tr - pointer to store the trailer character> + *> + * In case of success 0 is returned and buf and size are updated with> + * the amount of bytes read. If tr is non NULL and a trailing> + * character exist (size is non zero after returning from this> + * function) tr is updated with the trailing character.> + */> +static int proc_get_long(char **buf, size_t *size,> +             unsigned long *val, bool *neg,> +             const char *perm_tr, unsigned perm_tr_len, char *tr)> +{> +    int len;> +    char *p, tmp[TMPBUFLEN];> +> +    if (!*size)> +        return -EINVAL;> +> +    len = *size;> +    if (len > TMPBUFLEN-1)> +        len = TMPBUFLEN-1;> +> +    memcpy(tmp, *buf, len);> +> +    tmp[len] = 0;> +    p = tmp;> +    if (*p == '-' && *size > 1) {> +        *neg = 1; As neg is bool*, you should use true and false instead of 1 and 0. > +        p++;> +    } else> +        *neg = 0;> +    if (!isdigit(*p))> +        return -EINVAL;> +> +    *val = simple_strtoul(p, &p, 0);>> -static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp,> +    len = p - tmp;> +> +    /* We don't know if the next char is whitespace thus we may accept> +     * invalid integers (e.g. 1234...a) or two integers instead of one> +     * (e.g. 123...1). So lets not allow such large numbers. */> +    if (len == TMPBUFLEN - 1)> +        return -EINVAL;> +> +    if (len < *size && perm_tr_len && !memchr(perm_tr, *p, perm_tr_len))> +        return -EINVAL;> +> +    if (tr && (len < *size))> +        *tr = *p;> +> +    *buf += len;> +    *size -= len;> +> +    return 0;> +}> +> +/**> + * proc_put_long - coverts an integer to a decimal ASCII formated string> + *> + * @buf - the user buffer> + * @size - the size of the user buffer> + * @val - the integer to be converted> + * @neg - sign of the number, %TRUE for negative> + *> + * In case of success 0 is returned and buf and size are updated with> + * the amount of bytes read.> + */> +static int proc_put_long(void __user **buf, size_t *size, unsigned long val,> +             bool neg)> +{> +    int len;> +    char tmp[TMPBUFLEN], *p = tmp;> +> +    sprintf(p, "%s%lu", neg ? "-" : "", val);> +    len = strlen(tmp);> +    if (len > *size)> +        len = *size;> +    if (copy_to_user(*buf, tmp, len))> +        return -EFAULT;> +    *size -= len;> +    *buf += len;> +    return 0;> +}> +#undef TMPBUFLEN> +> +static int proc_put_char(void __user **buf, size_t *size, char c)> +{> +    if (*size) {> +        char __user **buffer = (char __user **)buf;> +        if (put_user(c, *buffer))> +            return -EFAULT;> +        (*size)--, (*buffer)++;> +        *buf = *buffer;> +    }> +    return 0;> +}> +> +static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,>                 int *valp,>                 int write, void *data)>  {> @@ -2050,7 +2164,7 @@ static int do_proc_dointvec_conv(int *ne>     } else {>         int val = *valp;>         if (val < 0) {> -            *negp = -1;> +            *negp = 1;>             *lvalp = (unsigned long)-val;>         } else {>             *negp = 0;> @@ -2060,23 +2174,21 @@ static int do_proc_dointvec_conv(int *ne>     return 0;>  }>> +static const char proc_wspace_sep[] = { ' ', '\t', '\n' };> +>  static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,>          int write, void __user *buffer,>          size_t *lenp, loff_t *ppos,> -         int (*conv)(int *negp, unsigned long *lvalp, int *valp,> +         int (*conv)(bool *negp, unsigned long *lvalp, int *valp,>                int write, void *data),>          void *data)>  {> -#define TMPBUFLEN 21> -    int *i, vleft, first = 1, neg;> -    unsigned long lval;> -    size_t left, len;> +    int *i, vleft, first = 1, err = 0;> +    unsigned long page = 0;> +    size_t left;> +    char *kbuf;>> -    char buf[TMPBUFLEN], *p;> -    char __user *s = buffer;> -> -    if (!tbl_data || !table->maxlen || !*lenp ||> -      (*ppos && !write)) {> +    if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {>         *lenp = 0;>         return 0;>     }> @@ -2088,89 +2200,69 @@ static int __do_proc_dointvec(void *tbl_>     if (!conv)>         conv = do_proc_dointvec_conv;>> +    if (write) {> +        if (left > PAGE_SIZE - 1)> +            left = PAGE_SIZE - 1;> +        page = __get_free_page(GFP_TEMPORARY);> +        kbuf = (char *) page;> +        if (!kbuf)> +            return -ENOMEM;> +        if (copy_from_user(kbuf, buffer, left)) {> +            err = -EFAULT;> +            goto free;> +        }> +        kbuf[left] = 0;> +    }> +>     for (; left && vleft--; i++, first=0) {> -        if (write) {> -            while (left) {> -                char c;> -                if (get_user(c, s))> -                    return -EFAULT;> -                if (!isspace(c))> -                    break;> -                left--;> -                s++;> -            }> -            if (!left)> -                break;> -            neg = 0;> -            len = left;> -            if (len > sizeof(buf) - 1)> -                len = sizeof(buf) - 1;> -            if (copy_from_user(buf, s, len))> -                return -EFAULT;> -            buf[len] = 0;> -            p = buf;> -            if (*p == '-' && left > 1) {> -                neg = 1;> -                p++;> -            }> -            if (*p < '0' || *p > '9')> -                break;> +        unsigned long lval;> +        bool neg;>> -            lval = simple_strtoul(p, &p, 0);> +        if (write) {> +            left -= proc_skip_spaces(&kbuf);>> -            len = p-buf;> -            if ((len < left) && *p && !isspace(*p))> +            err = proc_get_long(&kbuf, &left, &lval, &neg,> +                       proc_wspace_sep,> +                       sizeof(proc_wspace_sep), NULL);> +            if (err)>                 break;> -            s += len;> -            left -= len;> -> -            if (conv(&neg, &lval, i, 1, data))> +            if (conv(&neg, &lval, i, 1, data)) {> +                err = -EINVAL;>                 break;> +            }>         } else {> -            p = buf;> +            if (conv(&neg, &lval, i, 0, data)) {> +                err = -EINVAL;> +                break;> +            }>             if (!first)> -                *p++ = '\t';> -> -            if (conv(&neg, &lval, i, 0, data))> +                err = proc_put_char(&buffer, &left, '\t');> +            if (err)> +                break;> +            err = proc_put_long(&buffer, &left, lval, neg);> +            if (err)>                 break;> -> -            sprintf(p, "%s%lu", neg ? "-" : "", lval);> -            len = strlen(buf);> -            if (len > left)> -                len = left;> -            if(copy_to_user(s, buf, len))> -                return -EFAULT;> -            left -= len;> -            s += len;>         }>     }>> -    if (!write && !first && left) {> -        if(put_user('\n', s))> -            return -EFAULT;> -        left--, s++;> -    }> +    if (!write && !first && left && !err)> +        err = proc_put_char(&buffer, &left, '\n');> +    if (write && !err)> +        left -= proc_skip_spaces(&kbuf);> +free:>     if (write) {> -        while (left) {> -            char c;> -            if (get_user(c, s++))> -                return -EFAULT;> -            if (!isspace(c))> -                break;> -            left--;> -        }> +        free_page(page);> +        if (first)> +            return err ? : -EINVAL;>     }> -    if (write && first)> -        return -EINVAL;>     *lenp -= left;>     *ppos += *lenp;> -    return 0;> -#undef TMPBUFLEN> +    return err;>  }>>  static int do_proc_dointvec(struct ctl_table *table, int write,>          void __user *buffer, size_t *lenp, loff_t *ppos,> -         int (*conv)(int *negp, unsigned long *lvalp, int *valp,> +         int (*conv)(bool *negp, unsigned long *lvalp, int *valp,>                int write, void *data),>          void *data)>  {> @@ -2238,8 +2330,8 @@ struct do_proc_dointvec_minmax_conv_para>     int *max;>  };>> -static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp,> -                    int *valp,> +static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,> +                    int *valp,>                     int write, void *data)>  {>     struct do_proc_dointvec_minmax_conv_param *param = data;> @@ -2252,7 +2344,7 @@ static int do_proc_dointvec_minmax_conv(>     } else {>         int val = *valp;>         if (val < 0) {> -            *negp = -1;> +            *negp = 1;>             *lvalp = (unsigned long)-val;>         } else {>             *negp = 0;> @@ -2295,102 +2387,78 @@ static int __do_proc_doulongvec_minmax(v>                   unsigned long convmul,>                   unsigned long convdiv)>  {> -#define TMPBUFLEN 21> -    unsigned long *i, *min, *max, val;> -    int vleft, first=1, neg;> -    size_t len, left;> -    char buf[TMPBUFLEN], *p;> -    char __user *s = buffer;> -> -    if (!data || !table->maxlen || !*lenp ||> -      (*ppos && !write)) {> +    unsigned long *i, *min, *max;> +    int vleft, first = 1, err = 0;> +    unsigned long page = 0;> +    size_t left;> +    char *kbuf;> +> +    if (!data || !table->maxlen || !*lenp || (*ppos && !write)) {>         *lenp = 0;>         return 0;>     }> -> +>     i = (unsigned long *) data;>     min = (unsigned long *) table->extra1;>     max = (unsigned long *) table->extra2;>     vleft = table->maxlen / sizeof(unsigned long);>     left = *lenp;> -> +> +    if (write) {> +        if (left > PAGE_SIZE - 1)> +            left = PAGE_SIZE - 1;> +        page = __get_free_page(GFP_TEMPORARY);> +        kbuf = (char *) page;> +        if (!kbuf)> +            return -ENOMEM;> +        if (copy_from_user(kbuf, buffer, left)) {> +            err = -EFAULT;> +            goto free;> +        }> +        kbuf[left] = 0;> +    }> +>     for (; left && vleft--; i++, min++, max++, first=0) {> +        unsigned long val;> +>         if (write) {> -            while (left) {> -                char c;> -                if (get_user(c, s))> -                    return -EFAULT;> -                if (!isspace(c))> -                    break;> -                left--;> -                s++;> -            }> -            if (!left)> -                break;> -            neg = 0;> -            len = left;> -            if (len > TMPBUFLEN-1)> -                len = TMPBUFLEN-1;> -            if (copy_from_user(buf, s, len))> -                return -EFAULT;> -            buf[len] = 0;> -            p = buf;> -            if (*p == '-' && left > 1) {> -                neg = 1;> -                p++;> -            }> -            if (*p < '0' || *p > '9')> -                break;> -            val = simple_strtoul(p, &p, 0) * convmul / convdiv ;> -            len = p-buf;> -            if ((len < left) && *p && !isspace(*p))> +            bool neg;> +> +            left -= proc_skip_spaces(&kbuf);> +> +            err = proc_get_long(&kbuf, &left, &val, &neg,> +                       proc_wspace_sep,> +                       sizeof(proc_wspace_sep), NULL);> +            if (err)>                 break;>             if (neg)> -                val = -val;> -            s += len;> -            left -= len;> -> -            if(neg)>                 continue;>             if ((min && val < *min) || (max && val > *max))>                 continue;>             *i = val;>         } else {> -            p = buf;> +            val = convdiv * (*i) / convmul;>             if (!first)> -                *p++ = '\t';> -            sprintf(p, "%lu", convdiv * (*i) / convmul);> -            len = strlen(buf);> -            if (len > left)> -                len = left;> -            if(copy_to_user(s, buf, len))> -                return -EFAULT;> -            left -= len;> -            s += len;> +                err = proc_put_char(&buffer, &left, '\t');> +            err = proc_put_long(&buffer, &left, val, false);> +            if (err)> +                break;>         }>     }>> -    if (!write && !first && left) {> -        if(put_user('\n', s))> -            return -EFAULT;> -        left--, s++;> -    }> +    if (!write && !first && left && !err)> +        err = proc_put_char(&buffer, &left, '\n');> +    if (write && !err)> +        left -= proc_skip_spaces(&kbuf);> +free:>     if (write) {> -        while (left) {> -            char c;> -            if (get_user(c, s++))> -                return -EFAULT;> -            if (!isspace(c))> -                break;> -            left--;> -        }> +        free_page(page);> +        if (first)> +            return err ? : -EINVAL;>     }> -    if (write && first)> -        return -EINVAL;>     *lenp -= left;>     *ppos += *lenp;> -    return 0;> -#undef TMPBUFLEN> +    return err;>  }>>  static int do_proc_doulongvec_minmax(struct ctl_table *table, int write,> @@ -2451,7 +2519,7 @@ int proc_doulongvec_ms_jiffies_minmax(st>  }>>> -static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp,> +static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp,>                     int *valp,>                     int write, void *data)>  {> @@ -2463,7 +2531,7 @@ static int do_proc_dointvec_jiffies_conv>         int val = *valp;>         unsigned long lval;>         if (val < 0) {> -            *negp = -1;> +            *negp = 1;>             lval = (unsigned long)-val;>         } else {>             *negp = 0;> @@ -2474,7 +2542,7 @@ static int do_proc_dointvec_jiffies_conv>     return 0;>  }>> -static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp,> +static int do_proc_dointvec_userhz_jiffies_conv(bool *negp, unsigned long *lvalp,>                         int *valp,>                         int write, void *data)>  {> @@ -2486,7 +2554,7 @@ static int do_proc_dointvec_userhz_jiffi>         int val = *valp;>         unsigned long lval;>         if (val < 0) {> -            *negp = -1;> +            *negp = 1;>             lval = (unsigned long)-val;>         } else {>             *negp = 0;> @@ -2497,7 +2565,7 @@ static int do_proc_dointvec_userhz_jiffi>     return 0;>  }>> -static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp,> +static int do_proc_dointvec_ms_jiffies_conv(bool *negp, unsigned long *lvalp,>                       int *valp,>                       int write, void *data)>  {> @@ -2507,7 +2575,7 @@ static int do_proc_dointvec_ms_jiffies_c>         int val = *valp;>         unsigned long lval;>         if (val < 0) {> -            *negp = -1;> +            *negp = 1;>             lval = (unsigned long)-val;>         } else {>             *negp = 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> -- Regardsï¼Changli Gao(xiaosuo@gmail.com)ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 1/3] sysctl: refactor integer handling proc code @ 2010-04-30 22:49 ` Changli Gao 0 siblings, 0 replies; 24+ messages in thread From: Changli Gao @ 2010-04-30 22:49 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, Octavian Purdila, Eric Dumazet, penguin-kernel, netdev, Neil Horman, ebiederm, David Miller, adobriyan On Fri, Apr 30, 2010 at 4:25 PM, Amerigo Wang <amwang@redhat.com> wrote: > (Based on Octavian's work, and I modified a lot.) > > As we are about to add another integer handling proc function a little > bit of cleanup is in order: add a few helper functions to improve code > readability and decrease code duplication. > > In the process a bug is also fixed: if the user specifies a number > with more then 20 digits it will be interpreted as two integers > (e.g. 10000...13 will be interpreted as 100.... and 13). > > Behavior for EFAULT handling was changed as well. Previous to this > patch, when an EFAULT error occurred in the middle of a write > operation, although some of the elements were set, that was not > acknowledged to the user (by shorting the write and returning the > number of bytes accepted). EFAULT is now treated just like any other > errors by acknowledging the amount of bytes accepted. > > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> > Signed-off-by: WANG Cong <amwang@redhat.com> > Cc: Eric W. Biederman <ebiederm@xmission.com> > --- > > Index: linux-2.6/kernel/sysctl.c > =================================================================== > --- linux-2.6.orig/kernel/sysctl.c > +++ linux-2.6/kernel/sysctl.c > @@ -2040,8 +2040,122 @@ int proc_dostring(struct ctl_table *tabl > buffer, lenp, ppos); > } > > +static size_t proc_skip_spaces(char **buf) > +{ > + size_t ret; > + char *tmp = skip_spaces(*buf); > + ret = tmp - *buf; > + *buf = tmp; > + return ret; > +} > + > +#define TMPBUFLEN 22 > +/** > + * proc_get_long - reads an ASCII formated integer from a user buffer > + * > + * @buf - a kernel buffer > + * @size - size of the kernel buffer > + * @val - this is where the number will be stored > + * @neg - set to %TRUE if number is negative > + * @perm_tr - a vector which contains the allowed trailers > + * @perm_tr_len - size of the perm_tr vector > + * @tr - pointer to store the trailer character > + * > + * In case of success 0 is returned and buf and size are updated with > + * the amount of bytes read. If tr is non NULL and a trailing > + * character exist (size is non zero after returning from this > + * function) tr is updated with the trailing character. > + */ > +static int proc_get_long(char **buf, size_t *size, > + unsigned long *val, bool *neg, > + const char *perm_tr, unsigned perm_tr_len, char *tr) > +{ > + int len; > + char *p, tmp[TMPBUFLEN]; > + > + if (!*size) > + return -EINVAL; > + > + len = *size; > + if (len > TMPBUFLEN-1) > + len = TMPBUFLEN-1; > + > + memcpy(tmp, *buf, len); > + > + tmp[len] = 0; > + p = tmp; > + if (*p == '-' && *size > 1) { > + *neg = 1; As neg is bool*, you should use true and false instead of 1 and 0. > + p++; > + } else > + *neg = 0; > + if (!isdigit(*p)) > + return -EINVAL; > + > + *val = simple_strtoul(p, &p, 0); > > -static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp, > + len = p - tmp; > + > + /* We don't know if the next char is whitespace thus we may accept > + * invalid integers (e.g. 1234...a) or two integers instead of one > + * (e.g. 123...1). So lets not allow such large numbers. */ > + if (len == TMPBUFLEN - 1) > + return -EINVAL; > + > + if (len < *size && perm_tr_len && !memchr(perm_tr, *p, perm_tr_len)) > + return -EINVAL; > + > + if (tr && (len < *size)) > + *tr = *p; > + > + *buf += len; > + *size -= len; > + > + return 0; > +} > + > +/** > + * proc_put_long - coverts an integer to a decimal ASCII formated string > + * > + * @buf - the user buffer > + * @size - the size of the user buffer > + * @val - the integer to be converted > + * @neg - sign of the number, %TRUE for negative > + * > + * In case of success 0 is returned and buf and size are updated with > + * the amount of bytes read. > + */ > +static int proc_put_long(void __user **buf, size_t *size, unsigned long val, > + bool neg) > +{ > + int len; > + char tmp[TMPBUFLEN], *p = tmp; > + > + sprintf(p, "%s%lu", neg ? "-" : "", val); > + len = strlen(tmp); > + if (len > *size) > + len = *size; > + if (copy_to_user(*buf, tmp, len)) > + return -EFAULT; > + *size -= len; > + *buf += len; > + return 0; > +} > +#undef TMPBUFLEN > + > +static int proc_put_char(void __user **buf, size_t *size, char c) > +{ > + if (*size) { > + char __user **buffer = (char __user **)buf; > + if (put_user(c, *buffer)) > + return -EFAULT; > + (*size)--, (*buffer)++; > + *buf = *buffer; > + } > + return 0; > +} > + > +static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, > int *valp, > int write, void *data) > { > @@ -2050,7 +2164,7 @@ static int do_proc_dointvec_conv(int *ne > } else { > int val = *valp; > if (val < 0) { > - *negp = -1; > + *negp = 1; > *lvalp = (unsigned long)-val; > } else { > *negp = 0; > @@ -2060,23 +2174,21 @@ static int do_proc_dointvec_conv(int *ne > return 0; > } > > +static const char proc_wspace_sep[] = { ' ', '\t', '\n' }; > + > static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table, > int write, void __user *buffer, > size_t *lenp, loff_t *ppos, > - int (*conv)(int *negp, unsigned long *lvalp, int *valp, > + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, > int write, void *data), > void *data) > { > -#define TMPBUFLEN 21 > - int *i, vleft, first = 1, neg; > - unsigned long lval; > - size_t left, len; > + int *i, vleft, first = 1, err = 0; > + unsigned long page = 0; > + size_t left; > + char *kbuf; > > - char buf[TMPBUFLEN], *p; > - char __user *s = buffer; > - > - if (!tbl_data || !table->maxlen || !*lenp || > - (*ppos && !write)) { > + if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) { > *lenp = 0; > return 0; > } > @@ -2088,89 +2200,69 @@ static int __do_proc_dointvec(void *tbl_ > if (!conv) > conv = do_proc_dointvec_conv; > > + if (write) { > + if (left > PAGE_SIZE - 1) > + left = PAGE_SIZE - 1; > + page = __get_free_page(GFP_TEMPORARY); > + kbuf = (char *) page; > + if (!kbuf) > + return -ENOMEM; > + if (copy_from_user(kbuf, buffer, left)) { > + err = -EFAULT; > + goto free; > + } > + kbuf[left] = 0; > + } > + > for (; left && vleft--; i++, first=0) { > - if (write) { > - while (left) { > - char c; > - if (get_user(c, s)) > - return -EFAULT; > - if (!isspace(c)) > - break; > - left--; > - s++; > - } > - if (!left) > - break; > - neg = 0; > - len = left; > - if (len > sizeof(buf) - 1) > - len = sizeof(buf) - 1; > - if (copy_from_user(buf, s, len)) > - return -EFAULT; > - buf[len] = 0; > - p = buf; > - if (*p == '-' && left > 1) { > - neg = 1; > - p++; > - } > - if (*p < '0' || *p > '9') > - break; > + unsigned long lval; > + bool neg; > > - lval = simple_strtoul(p, &p, 0); > + if (write) { > + left -= proc_skip_spaces(&kbuf); > > - len = p-buf; > - if ((len < left) && *p && !isspace(*p)) > + err = proc_get_long(&kbuf, &left, &lval, &neg, > + proc_wspace_sep, > + sizeof(proc_wspace_sep), NULL); > + if (err) > break; > - s += len; > - left -= len; > - > - if (conv(&neg, &lval, i, 1, data)) > + if (conv(&neg, &lval, i, 1, data)) { > + err = -EINVAL; > break; > + } > } else { > - p = buf; > + if (conv(&neg, &lval, i, 0, data)) { > + err = -EINVAL; > + break; > + } > if (!first) > - *p++ = '\t'; > - > - if (conv(&neg, &lval, i, 0, data)) > + err = proc_put_char(&buffer, &left, '\t'); > + if (err) > + break; > + err = proc_put_long(&buffer, &left, lval, neg); > + if (err) > break; > - > - sprintf(p, "%s%lu", neg ? "-" : "", lval); > - len = strlen(buf); > - if (len > left) > - len = left; > - if(copy_to_user(s, buf, len)) > - return -EFAULT; > - left -= len; > - s += len; > } > } > > - if (!write && !first && left) { > - if(put_user('\n', s)) > - return -EFAULT; > - left--, s++; > - } > + if (!write && !first && left && !err) > + err = proc_put_char(&buffer, &left, '\n'); > + if (write && !err) > + left -= proc_skip_spaces(&kbuf); > +free: > if (write) { > - while (left) { > - char c; > - if (get_user(c, s++)) > - return -EFAULT; > - if (!isspace(c)) > - break; > - left--; > - } > + free_page(page); > + if (first) > + return err ? : -EINVAL; > } > - if (write && first) > - return -EINVAL; > *lenp -= left; > *ppos += *lenp; > - return 0; > -#undef TMPBUFLEN > + return err; > } > > static int do_proc_dointvec(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, loff_t *ppos, > - int (*conv)(int *negp, unsigned long *lvalp, int *valp, > + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, > int write, void *data), > void *data) > { > @@ -2238,8 +2330,8 @@ struct do_proc_dointvec_minmax_conv_para > int *max; > }; > > -static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp, > - int *valp, > +static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp, > + int *valp, > int write, void *data) > { > struct do_proc_dointvec_minmax_conv_param *param = data; > @@ -2252,7 +2344,7 @@ static int do_proc_dointvec_minmax_conv( > } else { > int val = *valp; > if (val < 0) { > - *negp = -1; > + *negp = 1; > *lvalp = (unsigned long)-val; > } else { > *negp = 0; > @@ -2295,102 +2387,78 @@ static int __do_proc_doulongvec_minmax(v > unsigned long convmul, > unsigned long convdiv) > { > -#define TMPBUFLEN 21 > - unsigned long *i, *min, *max, val; > - int vleft, first=1, neg; > - size_t len, left; > - char buf[TMPBUFLEN], *p; > - char __user *s = buffer; > - > - if (!data || !table->maxlen || !*lenp || > - (*ppos && !write)) { > + unsigned long *i, *min, *max; > + int vleft, first = 1, err = 0; > + unsigned long page = 0; > + size_t left; > + char *kbuf; > + > + if (!data || !table->maxlen || !*lenp || (*ppos && !write)) { > *lenp = 0; > return 0; > } > - > + > i = (unsigned long *) data; > min = (unsigned long *) table->extra1; > max = (unsigned long *) table->extra2; > vleft = table->maxlen / sizeof(unsigned long); > left = *lenp; > - > + > + if (write) { > + if (left > PAGE_SIZE - 1) > + left = PAGE_SIZE - 1; > + page = __get_free_page(GFP_TEMPORARY); > + kbuf = (char *) page; > + if (!kbuf) > + return -ENOMEM; > + if (copy_from_user(kbuf, buffer, left)) { > + err = -EFAULT; > + goto free; > + } > + kbuf[left] = 0; > + } > + > for (; left && vleft--; i++, min++, max++, first=0) { > + unsigned long val; > + > if (write) { > - while (left) { > - char c; > - if (get_user(c, s)) > - return -EFAULT; > - if (!isspace(c)) > - break; > - left--; > - s++; > - } > - if (!left) > - break; > - neg = 0; > - len = left; > - if (len > TMPBUFLEN-1) > - len = TMPBUFLEN-1; > - if (copy_from_user(buf, s, len)) > - return -EFAULT; > - buf[len] = 0; > - p = buf; > - if (*p == '-' && left > 1) { > - neg = 1; > - p++; > - } > - if (*p < '0' || *p > '9') > - break; > - val = simple_strtoul(p, &p, 0) * convmul / convdiv ; > - len = p-buf; > - if ((len < left) && *p && !isspace(*p)) > + bool neg; > + > + left -= proc_skip_spaces(&kbuf); > + > + err = proc_get_long(&kbuf, &left, &val, &neg, > + proc_wspace_sep, > + sizeof(proc_wspace_sep), NULL); > + if (err) > break; > if (neg) > - val = -val; > - s += len; > - left -= len; > - > - if(neg) > continue; > if ((min && val < *min) || (max && val > *max)) > continue; > *i = val; > } else { > - p = buf; > + val = convdiv * (*i) / convmul; > if (!first) > - *p++ = '\t'; > - sprintf(p, "%lu", convdiv * (*i) / convmul); > - len = strlen(buf); > - if (len > left) > - len = left; > - if(copy_to_user(s, buf, len)) > - return -EFAULT; > - left -= len; > - s += len; > + err = proc_put_char(&buffer, &left, '\t'); > + err = proc_put_long(&buffer, &left, val, false); > + if (err) > + break; > } > } > > - if (!write && !first && left) { > - if(put_user('\n', s)) > - return -EFAULT; > - left--, s++; > - } > + if (!write && !first && left && !err) > + err = proc_put_char(&buffer, &left, '\n'); > + if (write && !err) > + left -= proc_skip_spaces(&kbuf); > +free: > if (write) { > - while (left) { > - char c; > - if (get_user(c, s++)) > - return -EFAULT; > - if (!isspace(c)) > - break; > - left--; > - } > + free_page(page); > + if (first) > + return err ? : -EINVAL; > } > - if (write && first) > - return -EINVAL; > *lenp -= left; > *ppos += *lenp; > - return 0; > -#undef TMPBUFLEN > + return err; > } > > static int do_proc_doulongvec_minmax(struct ctl_table *table, int write, > @@ -2451,7 +2519,7 @@ int proc_doulongvec_ms_jiffies_minmax(st > } > > > -static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp, > +static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp, > int *valp, > int write, void *data) > { > @@ -2463,7 +2531,7 @@ static int do_proc_dointvec_jiffies_conv > int val = *valp; > unsigned long lval; > if (val < 0) { > - *negp = -1; > + *negp = 1; > lval = (unsigned long)-val; > } else { > *negp = 0; > @@ -2474,7 +2542,7 @@ static int do_proc_dointvec_jiffies_conv > return 0; > } > > -static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp, > +static int do_proc_dointvec_userhz_jiffies_conv(bool *negp, unsigned long *lvalp, > int *valp, > int write, void *data) > { > @@ -2486,7 +2554,7 @@ static int do_proc_dointvec_userhz_jiffi > int val = *valp; > unsigned long lval; > if (val < 0) { > - *negp = -1; > + *negp = 1; > lval = (unsigned long)-val; > } else { > *negp = 0; > @@ -2497,7 +2565,7 @@ static int do_proc_dointvec_userhz_jiffi > return 0; > } > > -static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp, > +static int do_proc_dointvec_ms_jiffies_conv(bool *negp, unsigned long *lvalp, > int *valp, > int write, void *data) > { > @@ -2507,7 +2575,7 @@ static int do_proc_dointvec_ms_jiffies_c > int val = *valp; > unsigned long lval; > if (val < 0) { > - *negp = -1; > + *negp = 1; > lval = (unsigned long)-val; > } else { > *negp = 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 > -- Regards, Changli Gao(xiaosuo@gmail.com) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Patch 1/3] sysctl: refactor integer handling proc code 2010-04-30 22:49 ` Changli Gao (?) @ 2010-05-05 3:02 ` Cong Wang -1 siblings, 0 replies; 24+ messages in thread From: Cong Wang @ 2010-05-05 3:02 UTC (permalink / raw) To: Changli Gao Cc: linux-kernel, Octavian Purdila, Eric Dumazet, penguin-kernel, netdev, Neil Horman, ebiederm, David Miller, adobriyan Changli Gao wrote: > On Fri, Apr 30, 2010 at 4:25 PM, Amerigo Wang <amwang@redhat.com> wrote: >> + if (*p == '-' && *size > 1) { >> + *neg = 1; > > As neg is bool*, you should use true and false instead of 1 and 0. > Yeah, I only corrected those lines that I touched, I should correct them all. Will fix. Thanks. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Patch v10 0/3] net: reserve ports for applications using fixed port numbers @ 2010-05-05 10:26 Amerigo Wang 2010-05-05 10:26 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang 0 siblings, 1 reply; 24+ messages in thread From: Amerigo Wang @ 2010-05-05 10:26 UTC (permalink / raw) To: linux-kernel Cc: Octavian Purdila, ebiederm, Eric Dumazet, penguin-kernel, netdev, Neil Horman, Amerigo Wang, xiaosuo, adobriyan, David Miller Changes from the previous version: - Use 'true' and 'false' for bool's; - Fix some coding style problems; - Allow appending lines to bitmap proc file so that it will be easier to add new bits. ------------------> This patch introduces /proc/sys/net/ipv4/ip_local_reserved_ports which allows users to reserve ports for third-party applications. The reserved ports will not be used by automatic port assignments (e.g. when calling connect() or bind() with port number 0). Explicit port allocation behavior is unchanged. There are still some miss behaviors with regard to proc parsing in odd invalid cases (for "40000\0-40001" all is acknowledged but only 40000 is accepted) but they are not easy to fix without changing the current "acknowledge how much we accepted" behavior. Because of that and because the same issues are present in the existing proc_dointvec code as well I don't think its worth holding the actual feature (port reservation) after such petty error recovery issues. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Patch 1/3] sysctl: refactor integer handling proc code 2010-05-05 10:26 [Patch v10 0/3] net: reserve ports for applications using fixed port numbers Amerigo Wang @ 2010-05-05 10:26 ` Amerigo Wang 0 siblings, 0 replies; 24+ messages in thread From: Amerigo Wang @ 2010-05-05 10:26 UTC (permalink / raw) To: linux-kernel Cc: Octavian Purdila, Eric Dumazet, penguin-kernel, netdev, Neil Horman, ebiederm, xiaosuo, David Miller, adobriyan, Amerigo Wang (Based on Octavian's work, and I modified a lot.) As we are about to add another integer handling proc function a little bit of cleanup is in order: add a few helper functions to improve code readability and decrease code duplication. In the process a bug is also fixed: if the user specifies a number with more then 20 digits it will be interpreted as two integers (e.g. 10000...13 will be interpreted as 100.... and 13). Behavior for EFAULT handling was changed as well. Previous to this patch, when an EFAULT error occurred in the middle of a write operation, although some of the elements were set, that was not acknowledged to the user (by shorting the write and returning the number of bytes accepted). EFAULT is now treated just like any other errors by acknowledging the amount of bytes accepted. Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> Signed-off-by: WANG Cong <amwang@redhat.com> Cc: Eric W. Biederman <ebiederm@xmission.com> --- Index: linux-2.6/kernel/sysctl.c =================================================================== --- linux-2.6.orig/kernel/sysctl.c +++ linux-2.6/kernel/sysctl.c @@ -2040,8 +2040,122 @@ int proc_dostring(struct ctl_table *tabl buffer, lenp, ppos); } +static size_t proc_skip_spaces(char **buf) +{ + size_t ret; + char *tmp = skip_spaces(*buf); + ret = tmp - *buf; + *buf = tmp; + return ret; +} + +#define TMPBUFLEN 22 +/** + * proc_get_long - reads an ASCII formated integer from a user buffer + * + * @buf - a kernel buffer + * @size - size of the kernel buffer + * @val - this is where the number will be stored + * @neg - set to %TRUE if number is negative + * @perm_tr - a vector which contains the allowed trailers + * @perm_tr_len - size of the perm_tr vector + * @tr - pointer to store the trailer character + * + * In case of success 0 is returned and buf and size are updated with + * the amount of bytes read. If tr is non NULL and a trailing + * character exist (size is non zero after returning from this + * function) tr is updated with the trailing character. + */ +static int proc_get_long(char **buf, size_t *size, + unsigned long *val, bool *neg, + const char *perm_tr, unsigned perm_tr_len, char *tr) +{ + int len; + char *p, tmp[TMPBUFLEN]; + + if (!*size) + return -EINVAL; + + len = *size; + if (len > TMPBUFLEN - 1) + len = TMPBUFLEN - 1; + + memcpy(tmp, *buf, len); + + tmp[len] = 0; + p = tmp; + if (*p == '-' && *size > 1) { + *neg = true; + p++; + } else + *neg = false; + if (!isdigit(*p)) + return -EINVAL; + + *val = simple_strtoul(p, &p, 0); -static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp, + len = p - tmp; + + /* We don't know if the next char is whitespace thus we may accept + * invalid integers (e.g. 1234...a) or two integers instead of one + * (e.g. 123...1). So lets not allow such large numbers. */ + if (len == TMPBUFLEN - 1) + return -EINVAL; + + if (len < *size && perm_tr_len && !memchr(perm_tr, *p, perm_tr_len)) + return -EINVAL; + + if (tr && (len < *size)) + *tr = *p; + + *buf += len; + *size -= len; + + return 0; +} + +/** + * proc_put_long - coverts an integer to a decimal ASCII formated string + * + * @buf - the user buffer + * @size - the size of the user buffer + * @val - the integer to be converted + * @neg - sign of the number, %TRUE for negative + * + * In case of success 0 is returned and buf and size are updated with + * the amount of bytes read. + */ +static int proc_put_long(void __user **buf, size_t *size, unsigned long val, + bool neg) +{ + int len; + char tmp[TMPBUFLEN], *p = tmp; + + sprintf(p, "%s%lu", neg ? "-" : "", val); + len = strlen(tmp); + if (len > *size) + len = *size; + if (copy_to_user(*buf, tmp, len)) + return -EFAULT; + *size -= len; + *buf += len; + return 0; +} +#undef TMPBUFLEN + +static int proc_put_char(void __user **buf, size_t *size, char c) +{ + if (*size) { + char __user **buffer = (char __user **)buf; + if (put_user(c, *buffer)) + return -EFAULT; + (*size)--, (*buffer)++; + *buf = *buffer; + } + return 0; +} + +static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2050,33 +2164,31 @@ static int do_proc_dointvec_conv(int *ne } else { int val = *valp; if (val < 0) { - *negp = -1; + *negp = true; *lvalp = (unsigned long)-val; } else { - *negp = 0; + *negp = false; *lvalp = (unsigned long)val; } } return 0; } +static const char proc_wspace_sep[] = { ' ', '\t', '\n' }; + static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos, - int (*conv)(int *negp, unsigned long *lvalp, int *valp, + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, int write, void *data), void *data) { -#define TMPBUFLEN 21 - int *i, vleft, first = 1, neg; - unsigned long lval; - size_t left, len; + int *i, vleft, first = 1, err = 0; + unsigned long page = 0; + size_t left; + char *kbuf; - char buf[TMPBUFLEN], *p; - char __user *s = buffer; - - if (!tbl_data || !table->maxlen || !*lenp || - (*ppos && !write)) { + if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) { *lenp = 0; return 0; } @@ -2088,89 +2200,69 @@ static int __do_proc_dointvec(void *tbl_ if (!conv) conv = do_proc_dointvec_conv; + if (write) { + if (left > PAGE_SIZE - 1) + left = PAGE_SIZE - 1; + page = __get_free_page(GFP_TEMPORARY); + kbuf = (char *) page; + if (!kbuf) + return -ENOMEM; + if (copy_from_user(kbuf, buffer, left)) { + err = -EFAULT; + goto free; + } + kbuf[left] = 0; + } + for (; left && vleft--; i++, first=0) { - if (write) { - while (left) { - char c; - if (get_user(c, s)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - s++; - } - if (!left) - break; - neg = 0; - len = left; - if (len > sizeof(buf) - 1) - len = sizeof(buf) - 1; - if (copy_from_user(buf, s, len)) - return -EFAULT; - buf[len] = 0; - p = buf; - if (*p == '-' && left > 1) { - neg = 1; - p++; - } - if (*p < '0' || *p > '9') - break; + unsigned long lval; + bool neg; - lval = simple_strtoul(p, &p, 0); + if (write) { + left -= proc_skip_spaces(&kbuf); - len = p-buf; - if ((len < left) && *p && !isspace(*p)) + err = proc_get_long(&kbuf, &left, &lval, &neg, + proc_wspace_sep, + sizeof(proc_wspace_sep), NULL); + if (err) break; - s += len; - left -= len; - - if (conv(&neg, &lval, i, 1, data)) + if (conv(&neg, &lval, i, 1, data)) { + err = -EINVAL; break; + } } else { - p = buf; + if (conv(&neg, &lval, i, 0, data)) { + err = -EINVAL; + break; + } if (!first) - *p++ = '\t'; - - if (conv(&neg, &lval, i, 0, data)) + err = proc_put_char(&buffer, &left, '\t'); + if (err) + break; + err = proc_put_long(&buffer, &left, lval, neg); + if (err) break; - - sprintf(p, "%s%lu", neg ? "-" : "", lval); - len = strlen(buf); - if (len > left) - len = left; - if(copy_to_user(s, buf, len)) - return -EFAULT; - left -= len; - s += len; } } - if (!write && !first && left) { - if(put_user('\n', s)) - return -EFAULT; - left--, s++; - } + if (!write && !first && left && !err) + err = proc_put_char(&buffer, &left, '\n'); + if (write && !err) + left -= proc_skip_spaces(&kbuf); +free: if (write) { - while (left) { - char c; - if (get_user(c, s++)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - } + free_page(page); + if (first) + return err ? : -EINVAL; } - if (write && first) - return -EINVAL; *lenp -= left; *ppos += *lenp; - return 0; -#undef TMPBUFLEN + return err; } static int do_proc_dointvec(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos, - int (*conv)(int *negp, unsigned long *lvalp, int *valp, + int (*conv)(bool *negp, unsigned long *lvalp, int *valp, int write, void *data), void *data) { @@ -2238,8 +2330,8 @@ struct do_proc_dointvec_minmax_conv_para int *max; }; -static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp, - int *valp, +static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp, + int *valp, int write, void *data) { struct do_proc_dointvec_minmax_conv_param *param = data; @@ -2252,10 +2344,10 @@ static int do_proc_dointvec_minmax_conv( } else { int val = *valp; if (val < 0) { - *negp = -1; + *negp = true; *lvalp = (unsigned long)-val; } else { - *negp = 0; + *negp = false; *lvalp = (unsigned long)val; } } @@ -2295,102 +2387,78 @@ static int __do_proc_doulongvec_minmax(v unsigned long convmul, unsigned long convdiv) { -#define TMPBUFLEN 21 - unsigned long *i, *min, *max, val; - int vleft, first=1, neg; - size_t len, left; - char buf[TMPBUFLEN], *p; - char __user *s = buffer; - - if (!data || !table->maxlen || !*lenp || - (*ppos && !write)) { + unsigned long *i, *min, *max; + int vleft, first = 1, err = 0; + unsigned long page = 0; + size_t left; + char *kbuf; + + if (!data || !table->maxlen || !*lenp || (*ppos && !write)) { *lenp = 0; return 0; } - + i = (unsigned long *) data; min = (unsigned long *) table->extra1; max = (unsigned long *) table->extra2; vleft = table->maxlen / sizeof(unsigned long); left = *lenp; - + + if (write) { + if (left > PAGE_SIZE - 1) + left = PAGE_SIZE - 1; + page = __get_free_page(GFP_TEMPORARY); + kbuf = (char *) page; + if (!kbuf) + return -ENOMEM; + if (copy_from_user(kbuf, buffer, left)) { + err = -EFAULT; + goto free; + } + kbuf[left] = 0; + } + for (; left && vleft--; i++, min++, max++, first=0) { + unsigned long val; + if (write) { - while (left) { - char c; - if (get_user(c, s)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - s++; - } - if (!left) - break; - neg = 0; - len = left; - if (len > TMPBUFLEN-1) - len = TMPBUFLEN-1; - if (copy_from_user(buf, s, len)) - return -EFAULT; - buf[len] = 0; - p = buf; - if (*p == '-' && left > 1) { - neg = 1; - p++; - } - if (*p < '0' || *p > '9') - break; - val = simple_strtoul(p, &p, 0) * convmul / convdiv ; - len = p-buf; - if ((len < left) && *p && !isspace(*p)) + bool neg; + + left -= proc_skip_spaces(&kbuf); + + err = proc_get_long(&kbuf, &left, &val, &neg, + proc_wspace_sep, + sizeof(proc_wspace_sep), NULL); + if (err) break; if (neg) - val = -val; - s += len; - left -= len; - - if(neg) continue; if ((min && val < *min) || (max && val > *max)) continue; *i = val; } else { - p = buf; + val = convdiv * (*i) / convmul; if (!first) - *p++ = '\t'; - sprintf(p, "%lu", convdiv * (*i) / convmul); - len = strlen(buf); - if (len > left) - len = left; - if(copy_to_user(s, buf, len)) - return -EFAULT; - left -= len; - s += len; + err = proc_put_char(&buffer, &left, '\t'); + err = proc_put_long(&buffer, &left, val, false); + if (err) + break; } } - if (!write && !first && left) { - if(put_user('\n', s)) - return -EFAULT; - left--, s++; - } + if (!write && !first && left && !err) + err = proc_put_char(&buffer, &left, '\n'); + if (write && !err) + left -= proc_skip_spaces(&kbuf); +free: if (write) { - while (left) { - char c; - if (get_user(c, s++)) - return -EFAULT; - if (!isspace(c)) - break; - left--; - } + free_page(page); + if (first) + return err ? : -EINVAL; } - if (write && first) - return -EINVAL; *lenp -= left; *ppos += *lenp; - return 0; -#undef TMPBUFLEN + return err; } static int do_proc_doulongvec_minmax(struct ctl_table *table, int write, @@ -2451,7 +2519,7 @@ int proc_doulongvec_ms_jiffies_minmax(st } -static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2463,10 +2531,10 @@ static int do_proc_dointvec_jiffies_conv int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = true; lval = (unsigned long)-val; } else { - *negp = 0; + *negp = false; lval = (unsigned long)val; } *lvalp = lval / HZ; @@ -2474,7 +2542,7 @@ static int do_proc_dointvec_jiffies_conv return 0; } -static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_userhz_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2486,10 +2554,10 @@ static int do_proc_dointvec_userhz_jiffi int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = true; lval = (unsigned long)-val; } else { - *negp = 0; + *negp = false; lval = (unsigned long)val; } *lvalp = jiffies_to_clock_t(lval); @@ -2497,7 +2565,7 @@ static int do_proc_dointvec_userhz_jiffi return 0; } -static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp, +static int do_proc_dointvec_ms_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { @@ -2507,10 +2575,10 @@ static int do_proc_dointvec_ms_jiffies_c int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = true; lval = (unsigned long)-val; } else { - *negp = 0; + *negp = false; lval = (unsigned long)val; } *lvalp = jiffies_to_msecs(lval); ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2010-05-05 10:27 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-04-09 10:10 [Patch v7 0/3] net: reserve ports for applications using fixed port numbers Amerigo Wang 2010-04-09 10:11 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang 2010-04-09 10:49 ` Changli Gao 2010-04-09 10:49 ` Changli Gao 2010-04-09 13:40 ` Octavian Purdila 2010-04-12 6:30 ` Cong Wang 2010-04-09 10:11 ` [Patch 2/3] sysctl: add proc_do_large_bitmap Amerigo Wang 2010-04-09 10:33 ` Changli Gao 2010-04-09 12:35 ` Octavian Purdila 2010-04-12 6:32 ` Cong Wang 2010-04-09 10:11 ` [Patch 3/3] net: reserve ports for applications using fixed port numbers Amerigo Wang 2010-04-09 13:21 ` Tetsuo Handa 2010-04-12 6:52 ` Cong Wang 2010-04-12 7:06 ` Tetsuo Handa 2010-04-12 10:03 [Patch v8 0/3] " Amerigo Wang 2010-04-12 10:04 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang 2010-04-12 10:18 ` Alexey Dobriyan 2010-04-13 11:18 ` Alexey Dobriyan 2010-04-12 10:18 ` Alexey Dobriyan 2010-04-13 7:35 ` Cong Wang 2010-04-30 8:25 [Patch v9 0/3] net: reserve ports for applications using fixed port numbers Amerigo Wang 2010-04-30 8:25 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang 2010-04-30 22:49 ` Changli Gao 2010-04-30 22:49 ` Changli Gao 2010-05-05 3:02 ` Cong Wang 2010-05-05 10:26 [Patch v10 0/3] net: reserve ports for applications using fixed port numbers Amerigo Wang 2010-05-05 10:26 ` [Patch 1/3] sysctl: refactor integer handling proc code Amerigo Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.