Re: git: 099a81a4173b - main - linprocfs: Add support for proc/sysvipc/{msg,sem,shm}

On Sun, May 12, 2024 at 5:36 AM Konstantin Belousov <kostikbel@gmail.com>
wrote:

> On Sat, May 11, 2024 at 07:39:18PM +0000, Warner Losh wrote:
> > The branch main has been updated by imp:
> >
> > URL:
> https://cgit.FreeBSD.org/src/commit/?id=099a81a4173bc5b121e50d4e27ea5fafdda8475b
> >
> > commit 099a81a4173bc5b121e50d4e27ea5fafdda8475b
> > Author:     Ricardo Branco <rbranco@suse.de>
> > AuthorDate: 2024-05-04 13:38:20 +0000
> > Commit:     Warner Losh <imp@FreeBSD.org>
> > CommitDate: 2024-05-11 19:37:47 +0000
> >
> >     linprocfs: Add support for proc/sysvipc/{msg,sem,shm}
> >
> >     Signed-off-by: Ricardo Branco <rbranco@suse.de>
> >     Reviewed by: imp
> >     Pull Request: https://github.com/freebsd/freebsd-src/pull/1218
> > ---
> >  sys/compat/linprocfs/linprocfs.c | 182
> +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 182 insertions(+)
> >
> > diff --git a/sys/compat/linprocfs/linprocfs.c
> b/sys/compat/linprocfs/linprocfs.c
> > index 391d5f679ee5..a877d4065c18 100644
> > --- a/sys/compat/linprocfs/linprocfs.c
> > +++ b/sys/compat/linprocfs/linprocfs.c
> > @@ -126,6 +126,9 @@
> >  #define P2K(x) ((x) << (PAGE_SHIFT - 10))            /* pages to kbytes
> */
> >  #define TV2J(x)      ((x)->tv_sec * 100UL + (x)->tv_usec / 10000)
> >
> > +/* Value defined in sys/kern/sysv_shm.c */
> > +#define SHMSEG_ALLOCATED     0x0800
> > +
> >  /**
> >   * @brief Mapping of ki_stat in struct kinfo_proc to the linux state
> >   *
> > @@ -2092,6 +2095,176 @@ linprocfs_domax_map_cnt(PFS_FILL_ARGS)
> >       return (0);
> >  }
> >
> > +/*
> > + * Filler function for proc/sysvipc/msg
> > + */
> > +static int
> > +linprocfs_dosysvipc_msg(PFS_FILL_ARGS)
> > +{
> > +     struct msqid_kernel *msqids;
> > +     u_long id, msgmni;
> > +     size_t sz;
> > +     int error;
> > +
> > +     sbuf_printf(sb,
> > +         "%10s %10s %4s  %10s %10s %5s %5s %5s %5s %5s %5s %10s %10s
> %10s\n",
> > +         "key", "msqid", "perms", "cbytes", "qnum", "lspid", "lrpid",
> > +         "uid", "gid", "cuid", "cgid", "stime", "rtime", "ctime");
> > +
> > +again:
> > +     msgmni = msginfo.msgmni;
> > +     sz = sizeof(struct msqid_kernel) * msgmni;
> > +     msqids = malloc(sz, M_TEMP, M_NOWAIT);
> Why M_NOWAIT?  What does prevent us from waiting there?
>

Oh, I missed that in my review. It should just be wait. You are right.


> > +     if (msqids == NULL)
> > +             return (ENOMEM);
> > +     if (msgmni != msginfo.msgmni) {
> What prevents msginfo.msgmni from changing again?  Otherwise, why this
> check
> is needed?
>
> (Same questions for other two similar places trimmed below).
>

I found other races that I commented on, but missed these somehow. I'd also
bucketed the PR as simple, not in need of closer review by others, which
I'll bias towards wider review more in the future.

I've backed this out, as well as my "fixes" to it. You are of course
correct on the uintmax_t thing... I'd internalized that rule only for
[u]intXX_t, but of course it applies across the board. I don't know what I
was thinking. I'll work with the submitter to tighten these things up,
providing additional interfaces if necessary. Should I add you to the
reviews once the preliminaries and mechanical issues are dealt with?

Warner