Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759929Ab0D3WuW (ORCPT ); Fri, 30 Apr 2010 18:50:22 -0400 Received: from mail-pz0-f204.google.com ([209.85.222.204]:56884 "EHLO mail-pz0-f204.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759018Ab0D3WuU (ORCPT ); Fri, 30 Apr 2010 18:50:20 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding; b=s3NgLhFh6KGxusbbrnw0jYsdwfjKg3a6XBSlIcZwdflCb1zpGb8cPzt6edikzmiy71 ctZ6O3MqJKXndXBlm9DClyW1E/Qg+1Y3th68Lacks0n4CrjkpYdFldaoR7+UL69wAkfc IfqEJTyW+gq1kCnGoAqh3QhlRNqXTN6kSNgeU= MIME-Version: 1.0 In-Reply-To: <20100430082925.5630.58453.sendpatchset@localhost.localdomain> References: <20100430082912.5630.82405.sendpatchset@localhost.localdomain> <20100430082925.5630.58453.sendpatchset@localhost.localdomain> From: Changli Gao Date: Sat, 1 May 2010 06:49:59 +0800 Message-ID: Subject: Re: [Patch 1/3] sysctl: refactor integer handling proc code To: Amerigo Wang Cc: linux-kernel@vger.kernel.org, Octavian Purdila , Eric Dumazet , penguin-kernel@i-love.sakura.ne.jp, netdev@vger.kernel.org, Neil Horman , ebiederm@xmission.com, David Miller , adobriyan@gmail.com Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by alpha.home.local id o3UMoamE011669 Content-Length: 23894 Lines: 10 On Fri, Apr 30, 2010 at 4:25 PM, Amerigo Wang 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 > Signed-off-by: WANG Cong > Cc: Eric W. Biederman > --->> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?