[KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Anne Thrax]

do_task_stat() was using too much stack
from make checkstack:
0x080eae08 do_task_stat:                                1752

So I took almost all the variables used in the function and made a new 
structure do_task_stat_vars, and dynamically allocated it in the function.
This is supposed to be a quick hack, and hopefully the authors will 
really do something about it.

Signed off by: Anne Thrax <foobarfoobarfoobar@gmail.com>

--- linux-2.6.17-rc3-git13/fs/proc/array.c      2006-05-07 
23:01:50.000000000 -0400
+++ linux-uml/fs/proc/array.c   2006-05-08 01:31:12.000000000 -0400
@@ -316,7 +316,7 @@

  static int do_task_stat(struct task_struct *task, char * buffer, int 
whole)
  {
-       unsigned long vsize, eip, esp, wchan = ~0UL;
+       /* unsigned long vsize, eip, esp, wchan = ~0UL;
         long priority, nice;
         int tty_pgrp = -1, tty_nr = 0;
         sigset_t sigign, sigcatch;
@@ -332,35 +332,49 @@
         unsigned long rsslim = 0;
         struct task_struct *t;
         char tcomm[sizeof(task->comm)];
+       */
+       struct do_task_stat_vars *vars;
+       vars = kmalloc(sizeof(struct do_task_stat_vars), GFP_KERNEL);
+       vars->vsize = vars->eip = vars->esp = vars->wchan = ~0UL;
+       vars->tty_pgrp = -1;
+       vars->tty_nr = 0;
+       vars->pgid = -1;
+       vars->sid = -1;
+       vars->num_threads = 0;
+       vars->cmin_flt = vars->cmaj_flt = vars->min_flt = vars->maj_flt 
= vars->rsslim = 0;
+       struct mm_struct *mm;
+       struct task_struct *t;
+       char tcomm[sizeof(task->comm)];
+       char state;

         state = *get_task_state(task);
-       vsize = eip = esp = 0;
+       vars->vsize = vars->eip = vars->esp = 0;
         mm = get_task_mm(task);
         if (mm) {
-               vsize = task_vsize(mm);
-               eip = KSTK_EIP(task);
-               esp = KSTK_ESP(task);
+               vars->vsize = task_vsize(mm);
+               vars->eip = KSTK_EIP(task);
+               vars->esp = KSTK_ESP(task);
         }

         get_task_comm(tcomm, task);

-       sigemptyset(&sigign);
-       sigemptyset(&sigcatch);
-       cutime = cstime = utime = stime = cputime_zero;
+       sigemptyset(&vars->sigign);
+       sigemptyset(&vars->sigcatch);
+       vars->cutime = vars->cstime = vars->utime = vars->stime = 
cputime_zero;
         read_lock(&tasklist_lock);
         if (task->sighand) {
                 spin_lock_irq(&task->sighand->siglock);
-               num_threads = atomic_read(&task->signal->count);
-               collect_sigign_sigcatch(task, &sigign, &sigcatch);
+               vars->num_threads = atomic_read(&task->signal->count);
+               collect_sigign_sigcatch(task, &vars->sigign, 
&vars->sigcatch);

                 /* add up live thread stats at the group level */
                 if (whole) {
                         t = task;
                         do {
-                               min_flt += t->min_flt;
-                               maj_flt += t->maj_flt;
-                               utime = cputime_add(utime, t->utime);
-                               stime = cputime_add(stime, t->stime);
+                               vars->min_flt += t->min_flt;
+                               vars->maj_flt += t->maj_flt;
+                               vars->utime = cputime_add(vars->utime, 
t->utime);
+                               vars->stime = cputime_add(vars->stime, 
t->stime);
                                 t = next_thread(t);
                         } while (t != task);
                 }
@@ -369,88 +383,88 @@
         }
         if (task->signal) {
                 if (task->signal->tty) {
-                       tty_pgrp = task->signal->tty->pgrp;
-                       tty_nr = 
new_encode_dev(tty_devnum(task->signal->tty));
+                       vars->tty_pgrp = task->signal->tty->pgrp;
+                       vars->tty_nr = 
new_encode_dev(tty_devnum(task->signal->tty));
                 }
-               pgid = process_group(task);
-               sid = task->signal->session;
-               cmin_flt = task->signal->cmin_flt;
-               cmaj_flt = task->signal->cmaj_flt;
-               cutime = task->signal->cutime;
-               cstime = task->signal->cstime;
-               rsslim = task->signal->rlim[RLIMIT_RSS].rlim_cur;
+               vars->pgid = process_group(task);
+               vars->sid = task->signal->session;
+               vars->cmin_flt = task->signal->cmin_flt;
+               vars->cmaj_flt = task->signal->cmaj_flt;
+               vars->cutime = task->signal->cutime;
+               vars->cstime = task->signal->cstime;
+               vars->rsslim = task->signal->rlim[RLIMIT_RSS].rlim_cur;
                 if (whole) {
-                       min_flt += task->signal->min_flt;
-                       maj_flt += task->signal->maj_flt;
-                       utime = cputime_add(utime, task->signal->utime);
-                       stime = cputime_add(stime, task->signal->stime);
+                       vars->min_flt += task->signal->min_flt;
+                       vars->maj_flt += task->signal->maj_flt;
+                       vars->utime = cputime_add(vars->utime, 
task->signal->utime);
+                       vars->stime = cputime_add(vars->stime, 
task->signal->stime);
                 }
         }
-       ppid = pid_alive(task) ? task->group_leader->real_parent->tgid : 0;
+       vars->ppid = pid_alive(task) ? 
task->group_leader->real_parent->tgid : 0;
         read_unlock(&tasklist_lock);

-       if (!whole || num_threads<2)
-               wchan = get_wchan(task);
+       if (!whole || vars->num_threads<2)
+               vars->wchan = get_wchan(task);
         if (!whole) {
-               min_flt = task->min_flt;
-               maj_flt = task->maj_flt;
-               utime = task->utime;
-               stime = task->stime;
+               vars->min_flt = task->min_flt;
+               vars->maj_flt = task->maj_flt;
+               vars->utime = task->utime;
+               vars->stime = task->stime;
         }

         /* scale priority and nice values from timeslices to -20..20 */
         /* to make it look like a "normal" Unix priority/nice value  */
-       priority = task_prio(task);
-       nice = task_nice(task);
+       vars->priority = task_prio(task);
+       vars->nice = task_nice(task);

         /* Temporary variable needed for gcc-2.96 */
         /* convert timespec -> nsec*/
-       start_time = (unsigned long long)task->start_time.tv_sec * 
NSEC_PER_SEC
+       vars->start_time = (unsigned long long)task->start_time.tv_sec * 
NSEC_PER_SEC
                                 + task->start_time.tv_nsec;
         /* convert nsec -> ticks */
-       start_time = nsec_to_clock_t(start_time);
+       vars->start_time = nsec_to_clock_t(vars->start_time);

-       res = sprintf(buffer,"%d (%s) %c %d %d %d %d %d %lu %lu \
+       vars->res = sprintf(buffer,"%d (%s) %c %d %d %d %d %d %lu %lu \
  %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu 
%lu \
  %lu %lu %lu %lu %lu %lu %lu %lu %d %d %lu %lu\n",
                 task->pid,
                 tcomm,
                 state,
-               ppid,
-               pgid,
-               sid,
-               tty_nr,
-               tty_pgrp,
+               vars->ppid,
+               vars->pgid,
+               vars->sid,
+               vars->tty_nr,
+               vars->tty_pgrp,
                 task->flags,
-               min_flt,
-               cmin_flt,
-               maj_flt,
-               cmaj_flt,
-               cputime_to_clock_t(utime),
-               cputime_to_clock_t(stime),
-               cputime_to_clock_t(cutime),
-               cputime_to_clock_t(cstime),
-               priority,
-               nice,
-               num_threads,
-               start_time,
-               vsize,
+               vars->min_flt,
+               vars->cmin_flt,
+               vars->maj_flt,
+               vars->cmaj_flt,
+               cputime_to_clock_t(vars->utime),
+               cputime_to_clock_t(vars->stime),
+               cputime_to_clock_t(vars->cutime),
+               cputime_to_clock_t(vars->cstime),
+               vars->priority,
+               vars->nice,
+               vars->num_threads,
+               vars->start_time,
+               vars->vsize,
                 mm ? get_mm_rss(mm) : 0,
-               rsslim,
+               vars->rsslim,
                 mm ? mm->start_code : 0,
                 mm ? mm->end_code : 0,
                 mm ? mm->start_stack : 0,
-               esp,
-               eip,
+               vars->esp,
+               vars->eip,
                 /* The signal information here is obsolete.
                  * It must be decimal for Linux 2.0 compatibility.
                  * Use /proc/#/status for real-time signals.
                  */
                 task->pending.signal.sig[0] & 0x7fffffffUL,
                 task->blocked.sig[0] & 0x7fffffffUL,
-               sigign      .sig[0] & 0x7fffffffUL,
-               sigcatch    .sig[0] & 0x7fffffffUL,
-               wchan,
+               vars->sigign      .sig[0] & 0x7fffffffUL,
+               vars->sigcatch    .sig[0] & 0x7fffffffUL,
+               vars->wchan,
                 0UL,
                 0UL,
                 task->exit_signal,
@@ -459,6 +473,9 @@
                 task->policy);
         if(mm)
                 mmput(mm);
+
+       int res = vars->res;
+       kfree(vars);
         return res;
  }

--- linux-2.6.17-rc3-git13/include/linux/proc_fs.h      2006-05-07 
23:01:54.000000000 -0400
+++ linux-uml/include/linux/proc_fs.h   2006-05-08 01:26:54.000000000 -0400
@@ -83,6 +83,25 @@
         loff_t offset;
  };

+/* do_task_stat() from fs/proc/array.c uses *WAY* too much stack
+ * I'll just make a struct containing all the variables in there
+ * and dynamicaly allocate it
+ * this is a quick hack, and someone should probably really do
+ * something about it
+ */
+
+struct do_task_stat_vars {
+       unsigned long vsize, eip, esp, wchan, cmin_flt, cmaj_flt,
+       min_flt, maj_flt, rsslim;
+       unsigned long long start_time;
+       long priority, nice;
+       int tty_pgrp, tty_nr, res, num_threads;
+       pid_t ppid, pgid, sid;
+       sigset_t sigign, sigcatch;
+       cputime_t cutime, cstime, utime, stime;
+};
+
+
  #ifdef CONFIG_PROC_FS

  extern struct proc_dir_entry proc_root;
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]

On 08.05.2006 [01:48:58 -0400], Anne Thrax wrote:
> do_task_stat() was using too much stack
> from make checkstack:
> 0x080eae08 do_task_stat:                                1752
> 
> So I took almost all the variables used in the function and made a new
> structure do_task_stat_vars, and dynamically allocated it in the
> function.  This is supposed to be a quick hack, and hopefully the
> authors will really do something about it.
> 
> Signed off by: Anne Thrax <foobarfoobarfoobar@gmail.com>

Signed-off-by, please.

And, I must ask, is your name actually Anne Thrax? If not, please use
your real name.

> --- linux-2.6.17-rc3-git13/fs/proc/array.c      2006-05-07 
> 23:01:50.000000000 -0400
> +++ linux-uml/fs/proc/array.c   2006-05-08 01:31:12.000000000 -0400
> @@ -316,7 +316,7 @@

Patch appears to be line-wrapped :(

>  static int do_task_stat(struct task_struct *task, char * buffer, int 
> whole)
>  {
> -       unsigned long vsize, eip, esp, wchan = ~0UL;
> +       /* unsigned long vsize, eip, esp, wchan = ~0UL;
>         long priority, nice;
>         int tty_pgrp = -1, tty_nr = 0;
>         sigset_t sigign, sigcatch;
> @@ -332,35 +332,49 @@
>         unsigned long rsslim = 0;
>         struct task_struct *t;
>         char tcomm[sizeof(task->comm)];
> +       */
> +       struct do_task_stat_vars *vars;
> +       vars = kmalloc(sizeof(struct do_task_stat_vars), GFP_KERNEL);

What happens if this kmalloc fails? Can do_task_stat() return ENOMEM?
What happens if it does -- will that change userspace in some way
(generally unacceptable)?

<snip>

> @@ -459,6 +473,9 @@
>                 task->policy);
>         if(mm)
>                 mmput(mm);
> +
> +       int res = vars->res;

I'm almost positive that compilation would have complained about
declaring a variable in the middle of a block. Did you compile-test?

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Anne Thrax]

> Signed-off-by, please.
> 
> And, I must ask, is your name actually Anne Thrax? If not, please use
> your real name.
>

I would rather not use my real name, as I am only 14 years old.

> 
> What happens if this kmalloc fails? Can do_task_stat() return ENOMEM?
> What happens if it does -- will that change userspace in some way
> (generally unacceptable)?

I think it's okay to return -ENOMEM...
It's a static function, that is only used by two other functions in fs/proc/array.c
I don't know who uses those functions, but I would assume that it would not
change userspace as it's just used for the proc filesystem. I would think that
that the proc filesystem just wouldn't work.
 
> I'm almost positive that compilation would have complained about
> declaring a variable in the middle of a block. Did you compile-test?

I removed res from struct do_task_stat_vars and declared it on the stack.
It did get yelly at me about being against C99 standards before, it compiles fine now.

--- linux-2.6.17-rc3-git13/fs/proc/array.c      2006-05-07 23:01:50.000000000 -0400
+++ linux-uml/fs/proc/array.c   2006-05-08 03:04:09.000000000 -0400
@@ -316,7 +316,7 @@

 static int do_task_stat(struct task_struct *task, char * buffer, int whole)
 {
-       unsigned long vsize, eip, esp, wchan = ~0UL;
+       /*unsigned long vsize, eip, esp, wchan = ~0UL;
        long priority, nice;
        int tty_pgrp = -1, tty_nr = 0;
        sigset_t sigign, sigcatch;
@@ -332,35 +332,54 @@
        unsigned long rsslim = 0;
        struct task_struct *t;
        char tcomm[sizeof(task->comm)];
+       */
+       struct do_task_stat_vars *vars;
+       vars = kmalloc(sizeof(struct do_task_stat_vars), GFP_KERNEL);
+
+       if(vars = NULL)
+               return -ENOMEM;
+
+       vars->vsize = vars->eip = vars->esp = vars->wchan = ~0UL;
+       vars->tty_pgrp = -1;
+       vars->tty_nr = 0;
+       vars->pgid = -1;
+       vars->sid = -1;
+       vars->num_threads = 0;
+       vars->cmin_flt = vars->cmaj_flt = vars->min_flt = vars->maj_flt = vars->rsslim = 0;
+       struct mm_struct *mm;
+       struct task_struct *t;
+       char tcomm[sizeof(task->comm)];
+       char state;
+       int res;

        state = *get_task_state(task);
-       vsize = eip = esp = 0;
+       vars->vsize = vars->eip = vars->esp = 0;
        mm = get_task_mm(task);
        if (mm) {
-               vsize = task_vsize(mm);
-               eip = KSTK_EIP(task);
-               esp = KSTK_ESP(task);
+               vars->vsize = task_vsize(mm);
+               vars->eip = KSTK_EIP(task);
+               vars->esp = KSTK_ESP(task);
        }

        get_task_comm(tcomm, task);

-       sigemptyset(&sigign);
-       sigemptyset(&sigcatch);
-       cutime = cstime = utime = stime = cputime_zero;
+       sigemptyset(&vars->sigign);
+       sigemptyset(&vars->sigcatch);
+       vars->cutime = vars->cstime = vars->utime = vars->stime = cputime_zero;
        read_lock(&tasklist_lock);
        if (task->sighand) {
                spin_lock_irq(&task->sighand->siglock);
-               num_threads = atomic_read(&task->signal->count);
-               collect_sigign_sigcatch(task, &sigign, &sigcatch);
+               vars->num_threads = atomic_read(&task->signal->count);
+               collect_sigign_sigcatch(task, &vars->sigign, &vars->sigcatch);

                /* add up live thread stats at the group level */
                if (whole) {
                        t = task;
                        do {
-                               min_flt += t->min_flt;
-                               maj_flt += t->maj_flt;
-                               utime = cputime_add(utime, t->utime);
-                               stime = cputime_add(stime, t->stime);
+                               vars->min_flt += t->min_flt;
+                               vars->maj_flt += t->maj_flt;
+                               vars->utime = cputime_add(vars->utime, t->utime);
+                               vars->stime = cputime_add(vars->stime, t->stime);
                                t = next_thread(t);
                        } while (t != task);
                }
@@ -369,46 +388,46 @@
        }
        if (task->signal) {
                if (task->signal->tty) {
-                       tty_pgrp = task->signal->tty->pgrp;
-                       tty_nr = new_encode_dev(tty_devnum(task->signal->tty));
+                       vars->tty_pgrp = task->signal->tty->pgrp;
+                       vars->tty_nr = new_encode_dev(tty_devnum(task->signal->tty));
                }
-               pgid = process_group(task);
-               sid = task->signal->session;
-               cmin_flt = task->signal->cmin_flt;
-               cmaj_flt = task->signal->cmaj_flt;
-               cutime = task->signal->cutime;
-               cstime = task->signal->cstime;
-               rsslim = task->signal->rlim[RLIMIT_RSS].rlim_cur;
+               vars->pgid = process_group(task);
+               vars->sid = task->signal->session;
+               vars->cmin_flt = task->signal->cmin_flt;
+               vars->cmaj_flt = task->signal->cmaj_flt;
+               vars->cutime = task->signal->cutime;
+               vars->cstime = task->signal->cstime;
+               vars->rsslim = task->signal->rlim[RLIMIT_RSS].rlim_cur;
                if (whole) {
-                       min_flt += task->signal->min_flt;
-                       maj_flt += task->signal->maj_flt;
-                       utime = cputime_add(utime, task->signal->utime);
-                       stime = cputime_add(stime, task->signal->stime);
+                       vars->min_flt += task->signal->min_flt;
+                       vars->maj_flt += task->signal->maj_flt;
+                       vars->utime = cputime_add(vars->utime, task->signal->utime);
+                       vars->stime = cputime_add(vars->stime, task->signal->stime);
                }
        }
-       ppid = pid_alive(task) ? task->group_leader->real_parent->tgid : 0;
+       vars->ppid = pid_alive(task) ? task->group_leader->real_parent->tgid : 0;
        read_unlock(&tasklist_lock);

-       if (!whole || num_threads<2)
-               wchan = get_wchan(task);
+       if (!whole || vars->num_threads<2)
+               vars->wchan = get_wchan(task);
        if (!whole) {
-               min_flt = task->min_flt;
-               maj_flt = task->maj_flt;
-               utime = task->utime;
-               stime = task->stime;
+               vars->min_flt = task->min_flt;
+               vars->maj_flt = task->maj_flt;
+               vars->utime = task->utime;
+               vars->stime = task->stime;
        }

        /* scale priority and nice values from timeslices to -20..20 */
        /* to make it look like a "normal" Unix priority/nice value  */
-       priority = task_prio(task);
-       nice = task_nice(task);
+       vars->priority = task_prio(task);
+       vars->nice = task_nice(task);

        /* Temporary variable needed for gcc-2.96 */
        /* convert timespec -> nsec*/
-       start_time = (unsigned long long)task->start_time.tv_sec * NSEC_PER_SEC
+       vars->start_time = (unsigned long long)task->start_time.tv_sec * NSEC_PER_SEC
                                + task->start_time.tv_nsec;
        /* convert nsec -> ticks */
-       start_time = nsec_to_clock_t(start_time);
+       vars->start_time = nsec_to_clock_t(vars->start_time);

        res = sprintf(buffer,"%d (%s) %c %d %d %d %d %d %lu %lu \
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
@@ -416,41 +435,41 @@
                task->pid,
                tcomm,
                state,
-               ppid,
-               pgid,
-               sid,
-               tty_nr,
-               tty_pgrp,
+               vars->ppid,
+               vars->pgid,
+               vars->sid,
+               vars->tty_nr,
+               vars->tty_pgrp,
                task->flags,
-               min_flt,
-               cmin_flt,
-               maj_flt,
-               cmaj_flt,
-               cputime_to_clock_t(utime),
-               cputime_to_clock_t(stime),
-               cputime_to_clock_t(cutime),
-               cputime_to_clock_t(cstime),
-               priority,
-               nice,
-               num_threads,
-               start_time,
-               vsize,
+               vars->min_flt,
+               vars->cmin_flt,
+               vars->maj_flt,
+               vars->cmaj_flt,
+               cputime_to_clock_t(vars->utime),
+               cputime_to_clock_t(vars->stime),
+               cputime_to_clock_t(vars->cutime),
+               cputime_to_clock_t(vars->cstime),
+               vars->priority,
+               vars->nice,
+               vars->num_threads,
+               vars->start_time,
+               vars->vsize,
                mm ? get_mm_rss(mm) : 0,
-               rsslim,
+               vars->rsslim,
                mm ? mm->start_code : 0,
                mm ? mm->end_code : 0,
                mm ? mm->start_stack : 0,
-               esp,
-               eip,
+               vars->esp,
+               vars->eip,
                /* The signal information here is obsolete.
                 * It must be decimal for Linux 2.0 compatibility.
                 * Use /proc/#/status for real-time signals.
                 */
                task->pending.signal.sig[0] & 0x7fffffffUL,
                task->blocked.sig[0] & 0x7fffffffUL,
-               sigign      .sig[0] & 0x7fffffffUL,
-               sigcatch    .sig[0] & 0x7fffffffUL,
-               wchan,
+               vars->sigign      .sig[0] & 0x7fffffffUL,
+               vars->sigcatch    .sig[0] & 0x7fffffffUL,
+               vars->wchan,
                0UL,
                0UL,
                task->exit_signal,
@@ -459,6 +478,8 @@
                task->policy);
        if(mm)
                mmput(mm);
+
+       kfree(vars);
        return res;
 }

--- linux-2.6.17-rc3-git13/include/linux/proc_fs.h      2006-05-07 23:01:54.000000000 -0400
+++ linux-uml/include/linux/proc_fs.h   2006-05-08 02:55:49.000000000 -0400
@@ -83,6 +83,25 @@
        loff_t offset;
 };

+/* do_task_stat() from fs/proc/array.c uses *WAY* too much stack
+ * I'll just make a struct containing all the variables in there
+ * and dynamicaly allocate it
+ * this is a quick hack, and someone should probably really do
+ * something about it
+ */
+
+struct do_task_stat_vars {
+       unsigned long vsize, eip, esp, wchan, cmin_flt, cmaj_flt,
+       min_flt, maj_flt, rsslim;
+       unsigned long long start_time;
+       long priority, nice;
+       int tty_pgrp, tty_nr, num_threads;
+       pid_t ppid, pgid, sid;
+       sigset_t sigign, sigcatch;
+       cputime_t cutime, cstime, utime, stime;
+};
+
+
 #ifdef CONFIG_PROC_FS

 extern struct proc_dir_entry proc_root;

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 3433 bytes --]

On 08.05.2006 [03:27:10 -0400], Anne Thrax wrote:
> >Signed-off-by, please.
> >
> >And, I must ask, is your name actually Anne Thrax? If not, please use
> >your real name.
> >
> 
> I would rather not use my real name, as I am only 14 years old.

I'm not sure I entirely understand how the one stems from the other, but
I think the spirit of the "personal information" part of the DCO
(Developer's Certificate of Origin, in Documentation/SubmittingPatches)
is something like full disclosure. Also, I wonder how, if at all, the
DCO can be enforced when it involves a minor. I'll defer to Greg on this
one... at least on whatever precedent exists, I don't expect a legal
decision, Greg :)

> >What happens if this kmalloc fails? Can do_task_stat() return ENOMEM?
> >What happens if it does -- will that change userspace in some way
> >(generally unacceptable)?
> 
> I think it's okay to return -ENOMEM...
> It's a static function, that is only used by two other functions in 
> fs/proc/array.c
> I don't know who uses those functions, but I would assume that it
> would not change userspace as it's just used for the proc filesystem.
> I would think that that the proc filesystem just wouldn't work.

Please see who uses those functions. Userspace can see mounted proc
filesystems, right? And more than likely can cat a file (or files) that
somehow exports some of this data, I'd think. If that data can now not
appear, but used to always appear with a large stack, then it's a
userspace regression and unacceptable. Please try and figure out what
this code is used for (hint: /proc/pid/stat would be my first guess),
and make sure everything still works.

> >I'm almost positive that compilation would have complained about
> >declaring a variable in the middle of a block. Did you compile-test?
> 
> I removed res from struct do_task_stat_vars and declared it on the
> stack.  It did get yelly at me about being against C99 standards
> before, it compiles fine now.

<snip>

> +       struct mm_struct *mm;
> +       struct task_struct *t;
> +       char tcomm[sizeof(task->comm)];
> +       char state;
> +       int res;

<snip>

Please just put this at the top of the corresponding block. It's against
accepted CodingStyle (although I'm not sure if it's explicitly so) to
put these declarations in the middle of the block.

<snip>

> +/* do_task_stat() from fs/proc/array.c uses *WAY* too much stack
> + * I'll just make a struct containing all the variables in there
> + * and dynamicaly allocate it
> + * this is a quick hack, and someone should probably really do
> + * something about it
> + */

Generally, we prefer comments of the form

/*
 * text ...
 */

Also, it would be good, probably, to indicate who the "I" is in the
comment -- it's pretty useless unless your name is somewhere in the
file. Or make it imperative ("Just make a  struct ..."), and let git
take care of tracking your name.

> +struct do_task_stat_vars {
> +       unsigned long vsize, eip, esp, wchan, cmin_flt, cmaj_flt,
> +       min_flt, maj_flt, rsslim;
> +       unsigned long long start_time;
> +       long priority, nice;
> +       int tty_pgrp, tty_nr, num_threads;
> +       pid_t ppid, pgid, sid;
> +       sigset_t sigign, sigcatch;
> +       cputime_t cutime, cstime, utime, stime;
> +};

That is one big struct :)

> +
> +

Extraneous newline insertion?

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Greg KH]

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]

On Mon, May 08, 2006 at 08:18:08AM -0700, Nishanth Aravamudan wrote:
> On 08.05.2006 [03:27:10 -0400], Anne Thrax wrote:
> > >Signed-off-by, please.
> > >
> > >And, I must ask, is your name actually Anne Thrax? If not, please use
> > >your real name.
> > >
> > 
> > I would rather not use my real name, as I am only 14 years old.
> 
> I'm not sure I entirely understand how the one stems from the other, but
> I think the spirit of the "personal information" part of the DCO
> (Developer's Certificate of Origin, in Documentation/SubmittingPatches)
> is something like full disclosure. Also, I wonder how, if at all, the
> DCO can be enforced when it involves a minor. I'll defer to Greg on this
> one... at least on whatever precedent exists, I don't expect a legal
> decision, Greg :)

Doesn't matter, we still need a real name.  It's not a legal issue but a
"trace the blame" issue.

thanks,

greg k-h

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

On 08.05.2006 [09:40:10 -0700], Greg KH wrote:
> On Mon, May 08, 2006 at 08:18:08AM -0700, Nishanth Aravamudan wrote:
> > On 08.05.2006 [03:27:10 -0400], Anne Thrax wrote:
> > > >Signed-off-by, please.
> > > >
> > > >And, I must ask, is your name actually Anne Thrax? If not, please use
> > > >your real name.
> > > >
> > > 
> > > I would rather not use my real name, as I am only 14 years old.
> > 
> > I'm not sure I entirely understand how the one stems from the other, but
> > I think the spirit of the "personal information" part of the DCO
> > (Developer's Certificate of Origin, in Documentation/SubmittingPatches)
> > is something like full disclosure. Also, I wonder how, if at all, the
> > DCO can be enforced when it involves a minor. I'll defer to Greg on this
> > one... at least on whatever precedent exists, I don't expect a legal
> > decision, Greg :)
> 
> Doesn't matter, we still need a real name.  It's not a legal issue but a
> "trace the blame" issue.

Ok, thanks for the information, Greg. Perhaps (although it may be
unnecessary), this should be added to SubmittingPatches?

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

On Mon, May 08, 2006 at 09:40:10AM -0700, Greg KH wrote:
> On Mon, May 08, 2006 at 08:18:08AM -0700, Nishanth Aravamudan wrote:
> > On 08.05.2006 [03:27:10 -0400], Anne Thrax wrote:
> > > I would rather not use my real name, as I am only 14 years old.
> > 
> > I'm not sure I entirely understand how the one stems from the other, but
> > I think the spirit of the "personal information" part of the DCO
> > (Developer's Certificate of Origin, in Documentation/SubmittingPatches)
> > is something like full disclosure. Also, I wonder how, if at all, the
> > DCO can be enforced when it involves a minor. I'll defer to Greg on this
> > one... at least on whatever precedent exists, I don't expect a legal
> > decision, Greg :)
> 
> Doesn't matter, we still need a real name.  It's not a legal issue but a
> "trace the blame" issue.

A pseudonym + email address performs acceptably for "trace the blame".
So I don't see how your two statements are related.


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Greg KH]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

On Mon, May 08, 2006 at 10:58:45AM -0600, Matthew Wilcox wrote:
> On Mon, May 08, 2006 at 09:40:10AM -0700, Greg KH wrote:
> > On Mon, May 08, 2006 at 08:18:08AM -0700, Nishanth Aravamudan wrote:
> > > On 08.05.2006 [03:27:10 -0400], Anne Thrax wrote:
> > > > I would rather not use my real name, as I am only 14 years old.
> > > 
> > > I'm not sure I entirely understand how the one stems from the other, but
> > > I think the spirit of the "personal information" part of the DCO
> > > (Developer's Certificate of Origin, in Documentation/SubmittingPatches)
> > > is something like full disclosure. Also, I wonder how, if at all, the
> > > DCO can be enforced when it involves a minor. I'll defer to Greg on this
> > > one... at least on whatever precedent exists, I don't expect a legal
> > > decision, Greg :)
> > 
> > Doesn't matter, we still need a real name.  It's not a legal issue but a
> > "trace the blame" issue.
> 
> A pseudonym + email address performs acceptably for "trace the blame".
> So I don't see how your two statements are related.

It's a matter of trust.  If you want to trust us to take your changes,
you need to show that you trust us enough to be willing to accept
recgonition for those changes.  By using a pseudonym, you are not
showing a whole lot of trust, and by using it, it take a whole lot more
effort to get that trust.

That being said, there are a few pseudonyms out there that I do trust,
just because they have been around for years and show that they are
trustworthy people.

thanks,

greg k-h

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

On Mon, May 08, 2006 at 10:49:32AM -0700, Greg KH wrote:
> On Mon, May 08, 2006 at 10:58:45AM -0600, Matthew Wilcox wrote:
> > On Mon, May 08, 2006 at 09:40:10AM -0700, Greg KH wrote:
> > > Doesn't matter, we still need a real name.  It's not a legal issue but a
> > > "trace the blame" issue.
> > 
> > A pseudonym + email address performs acceptably for "trace the blame".
> > So I don't see how your two statements are related.
> 
> It's a matter of trust.  If you want to trust us to take your changes,
> you need to show that you trust us enough to be willing to accept
> recgonition for those changes.  By using a pseudonym, you are not
> showing a whole lot of trust, and by using it, it take a whole lot more
> effort to get that trust.
> 
> That being said, there are a few pseudonyms out there that I do trust,
> just because they have been around for years and show that they are
> trustworthy people.

So ... there's no reason to turn down someone's changes, simply because
they use a pseudonym?  Thanks for clearing that up.

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Alexey Dobriyan]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

On Mon, May 08, 2006 at 01:48:58AM -0400, Anne Thrax wrote:
> do_task_stat() was using too much stack
> from make checkstack:
> 0x080eae08 do_task_stat:                                1752

Here it takes 10 times less stack.

	$ make checkstack | grep do_task_stat
	0xc0167add do_task_stat:                                172
	0xc01675b9 do_task_stat:                                152
	0xc0167af0 do_task_stat:                                152

I don't understand where such huge amount of space is wasted. Probably
"tcomm" is the biggest stack consumer of do_task_stat(). Others are just
longs and pointers. Is checkstack script is buggy?

> +/* do_task_stat() from fs/proc/array.c uses *WAY* too much stack
> + * I'll just make a struct containing all the variables in there
> + * and dynamicaly allocate it
> + * this is a quick hack, and someone should probably really do
> + * something about it
> + */
> +
> +struct do_task_stat_vars {
> +       unsigned long vsize, eip, esp, wchan, cmin_flt, cmaj_flt,
> +       min_flt, maj_flt, rsslim;
> +       unsigned long long start_time;
> +       long priority, nice;
> +       int tty_pgrp, tty_nr, res, num_threads;
> +       pid_t ppid, pgid, sid;
> +       sigset_t sigign, sigcatch;
> +       cputime_t cutime, cstime, utime, stime;
> +};

Well, this is ugly regardless.


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Randy.Dunlap]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

On Mon, 8 May 2006 09:40:10 -0700 Greg KH wrote:

> On Mon, May 08, 2006 at 08:18:08AM -0700, Nishanth Aravamudan wrote:
> > On 08.05.2006 [03:27:10 -0400], Anne Thrax wrote:
> > > >Signed-off-by, please.
> > > >
> > > >And, I must ask, is your name actually Anne Thrax? If not, please use
> > > >your real name.
> > > >
> > > 
> > > I would rather not use my real name, as I am only 14 years old.
> > 
> > I'm not sure I entirely understand how the one stems from the other, but
> > I think the spirit of the "personal information" part of the DCO
> > (Developer's Certificate of Origin, in Documentation/SubmittingPatches)
> > is something like full disclosure. Also, I wonder how, if at all, the
> > DCO can be enforced when it involves a minor. I'll defer to Greg on this
> > one... at least on whatever precedent exists, I don't expect a legal
> > decision, Greg :)
> 
> Doesn't matter, we still need a real name.  It's not a legal issue but a
> "trace the blame" issue.

The DCO is about rights and licenses.  Tracing the blame isn't
mentioned.

---
~Randy

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] linux-2.6.17-rc3-git13 fs/proc/array.c

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 1781 bytes --]

On 10.05.2006 [15:15:39 -0700], Randy.Dunlap wrote:
> On Mon, 8 May 2006 09:40:10 -0700 Greg KH wrote:
> 
> > On Mon, May 08, 2006 at 08:18:08AM -0700, Nishanth Aravamudan wrote:
> > > On 08.05.2006 [03:27:10 -0400], Anne Thrax wrote:
> > > > >Signed-off-by, please.
> > > > >
> > > > >And, I must ask, is your name actually Anne Thrax? If not, please use
> > > > >your real name.
> > > > >
> > > > 
> > > > I would rather not use my real name, as I am only 14 years old.
> > > 
> > > I'm not sure I entirely understand how the one stems from the
> > > other, but I think the spirit of the "personal information" part
> > > of the DCO (Developer's Certificate of Origin, in
> > > Documentation/SubmittingPatches) is something like full
> > > disclosure. Also, I wonder how, if at all, the DCO can be enforced
> > > when it involves a minor. I'll defer to Greg on this one... at
> > > least on whatever precedent exists, I don't expect a legal
> > > decision, Greg :)
> > 
> > Doesn't matter, we still need a real name.  It's not a legal issue
> > but a "trace the blame" issue.
> 
> The DCO is about rights and licenses.  Tracing the blame isn't
> mentioned.

So I guess there are two issues, both of which are still somewhat up in
the air.

1) Can a minor, or more generally a person who does not divulge a real
name, legitimately certify anything mentioned in the DCO? Specifically
wrt. the validity of any rights or licenses.

2) Do we require a real name, or a well-known/verified pseudonym, for
the Signed-off-by for keeping track of commits and "tracing the blame"?

A no to 1) or a yes to 2) are roughly equivalent for this particular
case, but maybe only a lawyer can really answer 1)...

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors