Date: Sun, 12 May 2024 09:28:29 -0600
From: Warner Losh <imp@bsdimp.com>
To: Konstantin Belousov <kostikbel@gmail.com>
Cc: Warner Losh <imp@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org
Subject: Re: git: 099a81a4173b - main - linprocfs: Add support for proc/sysvipc/{msg,sem,shm}
Message-ID: <CANCZdfrVMEm4=TfdwZEMzE_vtkn7RrchQdRzDAUmKk83SUrM2g@mail.gmail.com>
In-Reply-To: <ZkCplEevAyUZfjpR@kib.kiev.ua>
References: <202405111939.44BJdIE5045793@gitrepo.freebsd.org> <ZkCplEevAyUZfjpR@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--00000000000062fc600618436ce4 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sun, May 12, 2024 at 5:36=E2=80=AFAM Konstantin Belousov <kostikbel@gmai= l.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=3D099a81a4173bc5b121e50d4e27ea5fa= fdda8475b > > > > 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 kbyte= s > */ > > #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 =3D msginfo.msgmni; > > + sz =3D sizeof(struct msqid_kernel) * msgmni; > > + msqids =3D 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 =3D=3D NULL) > > + return (ENOMEM); > > + if (msgmni !=3D 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 --00000000000062fc600618436ce4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Sun, May 12, 2024 at 5:36=E2=80=AF= AM Konstantin Belousov <<a href=3D"mailto:kostikbel@gmail.com">kostikbel= @gmail.com</a>> wrote:<br></div><blockquote class=3D"gmail_quote" style= =3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding= -left:1ex">On Sat, May 11, 2024 at 07:39:18PM +0000, Warner Losh wrote:<br> > The branch main has been updated by imp:<br> > <br> > URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3D099a81a4173b= c5b121e50d4e27ea5fafdda8475b" rel=3D"noreferrer" target=3D"_blank">https://= cgit.FreeBSD.org/src/commit/?id=3D099a81a4173bc5b121e50d4e27ea5fafdda8475b<= /a><br> > <br> > commit 099a81a4173bc5b121e50d4e27ea5fafdda8475b<br> > Author:=C2=A0 =C2=A0 =C2=A0Ricardo Branco <<a href=3D"mailto:rbranc= o@suse.de" target=3D"_blank">rbranco@suse.de</a>><br> > AuthorDate: 2024-05-04 13:38:20 +0000<br> > Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org><br> > CommitDate: 2024-05-11 19:37:47 +0000<br> > <br> >=C2=A0 =C2=A0 =C2=A0linprocfs: Add support for proc/sysvipc/{msg,sem,sh= m}<br> >=C2=A0 =C2=A0 =C2=A0<br> >=C2=A0 =C2=A0 =C2=A0Signed-off-by: Ricardo Branco <<a href=3D"mailto= :rbranco@suse.de" target=3D"_blank">rbranco@suse.de</a>><br> >=C2=A0 =C2=A0 =C2=A0Reviewed by: imp<br> >=C2=A0 =C2=A0 =C2=A0Pull Request: <a href=3D"https://github.com/freebsd= /freebsd-src/pull/1218" rel=3D"noreferrer" target=3D"_blank">https://github= .com/freebsd/freebsd-src/pull/1218</a><br> > ---<br> >=C2=A0 sys/compat/linprocfs/linprocfs.c | 182 +++++++++++++++++++++++++= ++++++++++++++<br> >=C2=A0 1 file changed, 182 insertions(+)<br> > <br> > diff --git a/sys/compat/linprocfs/linprocfs.c b/sys/compat/linprocfs/l= inprocfs.c<br> > index 391d5f679ee5..a877d4065c18 100644<br> > --- a/sys/compat/linprocfs/linprocfs.c<br> > +++ b/sys/compat/linprocfs/linprocfs.c<br> > @@ -126,6 +126,9 @@<br> >=C2=A0 #define P2K(x) ((x) << (PAGE_SHIFT - 10))=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 /* pages to kbytes */<br> >=C2=A0 #define TV2J(x)=C2=A0 =C2=A0 =C2=A0 ((x)->tv_sec * 100UL + (x= )->tv_usec / 10000)<br> >=C2=A0 <br> > +/* Value defined in sys/kern/sysv_shm.c */<br> > +#define SHMSEG_ALLOCATED=C2=A0 =C2=A0 =C2=A00x0800<br> > +<br> >=C2=A0 /**<br> >=C2=A0 =C2=A0* @brief Mapping of ki_stat in struct kinfo_proc to the li= nux state<br> >=C2=A0 =C2=A0*<br> > @@ -2092,6 +2095,176 @@ linprocfs_domax_map_cnt(PFS_FILL_ARGS)<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0return (0);<br> >=C2=A0 }<br> >=C2=A0 <br> > +/*<br> > + * Filler function for proc/sysvipc/msg<br> > + */<br> > +static int<br> > +linprocfs_dosysvipc_msg(PFS_FILL_ARGS)<br> > +{<br> > +=C2=A0 =C2=A0 =C2=A0struct msqid_kernel *msqids;<br> > +=C2=A0 =C2=A0 =C2=A0u_long id, msgmni;<br> > +=C2=A0 =C2=A0 =C2=A0size_t sz;<br> > +=C2=A0 =C2=A0 =C2=A0int error;<br> > +<br> > +=C2=A0 =C2=A0 =C2=A0sbuf_printf(sb,<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"%10s %10s %4s=C2=A0 %10s %10s= %5s %5s %5s %5s %5s %5s %10s %10s %10s\n",<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"key", "msqid",= "perms", "cbytes", "qnum", "lspid"= , "lrpid",<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"uid", "gid", &= quot;cuid", "cgid", "stime", "rtime", &q= uot;ctime");<br> > +<br> > +again:<br> > +=C2=A0 =C2=A0 =C2=A0msgmni =3D msginfo.msgmni;<br> > +=C2=A0 =C2=A0 =C2=A0sz =3D sizeof(struct msqid_kernel) * msgmni;<br> > +=C2=A0 =C2=A0 =C2=A0msqids =3D malloc(sz, M_TEMP, M_NOWAIT);<br> Why M_NOWAIT?=C2=A0 What does prevent us from waiting there?<br></blockquot= e><div><br></div><div>Oh, I missed that in my review. It should just be wai= t. You are right.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" s= tyle=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);pad= ding-left:1ex"> > +=C2=A0 =C2=A0 =C2=A0if (msqids =3D=3D NULL)<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (ENOMEM);<br> > +=C2=A0 =C2=A0 =C2=A0if (msgmni !=3D msginfo.msgmni) {<br> What prevents msginfo.msgmni from changing again?=C2=A0 Otherwise, why this= check<br> is needed?<br> <br> (Same questions for other two similar places trimmed below).<br></blockquot= e><div><br></div><div>I found other races that I commented on, but missed t= hese 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 fut= ure.</div><div><br></div><div>I've backed this out, as well as my "= ;fixes" to it. You are of course correct on the uintmax_t thing... I&#= 39;d internalized that rule only for [u]intXX_t, but of course it applies a= cross the board. I don't know what I was thinking. I'll work with t= he submitter to tighten these things up, providing additional interfaces if= necessary. Should I add you to the reviews once the preliminaries and mech= anical issues are dealt with?</div><div><br></div><div>Warner</div><div><br= ></div></div></div> --00000000000062fc600618436ce4--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrVMEm4=TfdwZEMzE_vtkn7RrchQdRzDAUmKk83SUrM2g>