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

On Sun, May 12, 2024 at 09:28:29AM -0600, Warner Losh wrote:
> 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?

FWIW, I am not sure that the issues are worth the revert.  The time_t format
is definitely a mechanical/small problem.  Same for the M_NOWAIT/WAITOK.

The recheck is more problematic, I do agree.  Also, I realized one more
issue there: the data structures like msginfo are from sysvmsg.ko modules.
Then, the change make linprocfs depending on the sysv*.ko, but this is
not directly recorded with MODULE_DEPENDS().  It worked because GENERIC
compiles SysV support statically.