On Tue, Jan 18, 2022 at 11:29:54AM +0100, Jiri Slaby wrote:
> > +static const char * const task_state_array[] = {
> > + "running",
> > + "sleeping",
> > + "disk sleep",
> > + "stopped",
> > + "tracing stop",
> > + "dead",
> > + "zombie",
> > + "parked",
> > + "idle",
> > +};
Please no; don't add yet another one of these things. Can't you use the
one in fs/proc/array.c ?
> > +static inline struct task_struct *compare(struct task_struct *new,
> > + struct task_struct *old)
> > +{
> > + unsigned int ostate, nstate;
> > +
> > + if (old == NULL)
> > + return new;
> > +
> > + ostate = task_state_index(old);
> > + nstate = task_state_index(new);
> > +
> > + if (ostate == nstate) {
That's not an ordered set, please don't do that.
> > + if (old->start_time > new->start_time)
> > + return old;
> > + return new;
> > + }
> > +
> > + if (ostate < nstate)
> > + return old;
> > +
> > + return new;
> > +}
> > +static inline const char *get_task_state_name(struct task_struct *tsk)
>
> This definitely doesn't belong here. How do you ensure it matches the
> returned index also in the future. Put it along with task_index_to_char()?
> Or simply use task_state_to_char()?
>
> > +{
> > +
> > + int index;
> > +
> > + index = task_state_index(tsk);
> > + if (index > ARRAY_SIZE(task_state_array))
>
> Should be >=, or?
>
> > + return task_state_unknown;
> > + return task_state_array[index];
> > +}
*groan*.. that's very bad copy paste from fs/proc/array.c. There at east
there's a BUILD_BUG_ON() to make sure things are good.
> > +
> > +int n_tty_get_status(struct tty_struct *tty, char *msg, size_t *msglen)
> > +{
> > + unsigned long loadavg[3];
> > + uint64_t pcpu, cputime, wallclock;
> > + struct task_struct *p;
> > + struct rusage rusage;
> > + struct timespec64 utime, stime, rtime;
> > + char tname[TASK_COMM_LEN];
>
> How much stack did you consume in sum with its caller n_tty_status()?
>
> > + size_t len;
> > +
> > + if (tty == NULL)
> > + return -ENOTTY;
>
> How can this happen?
>
> > + get_avenrun(loadavg, FIXED_1/200, 0);
> > + len = scnprintf(msg + len, *msglen - len, "load: %lu.%02lu ",
> > + LOAD_INT(loadavg[0]), LOAD_FRAC(loadavg[0]));
> > +
> > + if (tty->ctrl.session == NULL) {
> > + len += scnprintf(msg + len, *msglen - len,
> > + "not a controlling terminal\n");
> > + goto out;
> > + }
> > +
> > + if (tty->ctrl.pgrp == NULL) {
> > + len += scnprintf(msg + len, *msglen - len,
> > + "no foreground process group\n");
> > + goto out;
> > + }
> > +
> > + p = pick_process(tty->ctrl.pgrp);
>
> Why is no lock needed?
>
> > + if (p == NULL) {
> > + len += scnprintf(msg + len, *msglen - len,
> > + "empty foreground process group\n");
> > + goto out;
> > + }
> > +
> > + get_task_comm(tname, p);
> > + getrusage(p, RUSAGE_BOTH, &rusage);
> > + wallclock = ktime_get_ns() - p->start_time;
> > +
> > + utime.tv_sec = rusage.ru_utime.tv_sec;
> > + utime.tv_nsec = rusage.ru_utime.tv_usec * NSEC_PER_USEC;
> > + stime.tv_sec = rusage.ru_stime.tv_sec;
> > + stime.tv_nsec = rusage.ru_stime.tv_usec * NSEC_PER_USEC;
> > + rtime = ns_to_timespec64(wallclock);
> > +
> > + cputime = timespec64_to_ns(&utime) + timespec64_to_ns(&stime);
> > + pcpu = div64_u64(cputime * 100, wallclock);
How is this number useful?
> > +
> > + len += scnprintf(msg + len, *msglen - len,
> > + /* task, PID, task state */
> > + "cmd: %s %d [%s] "
> > + /* rtime, utime, stime, %cpu, rss */
> > + "%llu.%02lur %llu.%02luu %llu.%02lus %llu%% %luk\n",
> > + tname, __get_pid(p, tty),
> > + (char *)get_task_state_name(p),
> > + rtime.tv_sec, frac_sec(rtime.tv_nsec),
> > + utime.tv_sec, frac_sec(utime.tv_nsec),
> > + stime.tv_sec, frac_sec(stime.tv_nsec),
> > + pcpu, getRSSk(p->mm));
>
> Why do you think p->mm is still alive (even after the getRSSk()'s check)? So
> no get_task_mm() needed?
>
> > +out:
> > + *msglen = len;
> > + return 0;
> > +}
Re lack of refcounting and locking, perhaps he's attemting a root hole?
