Re: [PATCH v2] ipc,shm: document new limits in the uapi header

[Davidlohr Bueso]

On Mon, 2014-05-12 at 09:44 +0200, Michael Kerrisk (man-pages) wrote:
> Hi Davidlohr,
> 
> On Sun, May 11, 2014 at 10:46 PM, Davidlohr Bueso <davidlohr@xxxxxx> wrote:
> > On Fri, 2014-05-09 at 10:44 +0200, Michael Kerrisk (man-pages) wrote:
> >> On Wed, May 7, 2014 at 9:17 PM, Davidlohr Bueso <davidlohr@xxxxxx> wrote:
> >> > This is useful in the future and allows users to
> >> > better understand the reasoning behind the changes.
> >> >
> >> > Signed-off-by: Davidlohr Bueso <davidlohr@xxxxxx>
> >> > ---
> >> >  include/uapi/linux/shm.h | 15 +++++++++------
> >> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
> >> > index 74e786d..3400b6e 100644
> >> > --- a/include/uapi/linux/shm.h
> >> > +++ b/include/uapi/linux/shm.h
> >> > @@ -8,17 +8,20 @@
> >> >  #endif
> >> >
> >> >  /*
> >> > - * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
> >> > - * be modified by sysctl.
> >> > + * SHMMNI, SHMMAX and SHMALL are the default upper limits which can be
> >> > + * modified by sysctl. Both SHMMAX and SHMALL have their default values
> >> > + * to the maximum limit which is as large as it can be without helping
> >> > + * userspace overflow the values. There is really nothing the kernel
> >> > + * can do to avoid this any further. It is therefore not advised to
> >> > + * make them any larger. These limits are suitable for both 32 and
> >> > + * 64-bit systems.
> >>
> >> I somehow find that text still rather impenetrable. What about this:
> >>
> >> SHMMNI, SHMMAX and SHMALL are default upper limits which can be
> >> modified by sysctl. The SHMMAX and SHMALL values have been chosen to
> >> be as large possible without facilitating scenarios where userspace
> >> causes overflows when adjusting the limits via operations of the form
> >> "retrieve current limit; add X; update limit". It is therefore not
> >> advised to make SHMMAX and SHMALL any larger. These limits are
> >> suitable for both 32 and 64-bit systems.
> >
> > I don't really have much preference, imho both read pretty much the
> > same, specially considering this is still code after all. If you guys
> > really prefer updating it, let me know and I'll send a v3. But perhaps
> > your text is a bit more suitable in the svipc manpage?
> 
> The problem is that part of your text is still broken grammatically In
> particular, the piece "Both SHMMAX and SHMALL have their default
> values to the maximum limit" at the very least lacks a word. That's
> what prompted me to propose the alternative, rather than just say
> "this is wrong"--and I thought that I might as well make a more
> thoroughgoing attempt at helping improve the text.
> 
> I agree that text something like this should land in the man page at
> some point, but as long as we're going to the trouble to improve the
> comments in the code, let's make them as good and helpful as we can.

Fair enough, and I trust your grammar corrections over mine ;) Thanks
for taking a closer look. I've added your text below.

8<------------------------------------------
From: Davidlohr Bueso <davidlohr@xxxxxx>
Subject: [PATCH v3] ipc,shm: document new limits in the uapi header

This is useful in the future and allows users to
better understand the reasoning behind the changes.

Signed-off-by: Davidlohr Bueso <davidlohr@xxxxxx>
---
 include/uapi/linux/shm.h | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
index 74e786d..1fbf24e 100644
--- a/include/uapi/linux/shm.h
+++ b/include/uapi/linux/shm.h
@@ -8,17 +8,20 @@
 #endif
 
 /*
- * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
- * be modified by sysctl.
+ * SHMMNI, SHMMAX and SHMALL are default upper limits which can be
+ * modified by sysctl. The SHMMAX and SHMALL values have been chosen to
+ * be as large possible without facilitating scenarios where userspace
+ * causes overflows when adjusting the limits via operations of the form
+ * "retrieve current limit; add X; update limit". It is therefore not
+ * advised to make SHMMAX and SHMALL any larger. These limits are
+ * suitable for both 32 and 64-bit systems.
  */
-
 #define SHMMIN 1			 /* min shared seg size (bytes) */
 #define SHMMNI 4096			 /* max num of segs system wide */
-#define SHMMAX (ULONG_MAX - (1L<<24))	 /* max shared seg size (bytes) */
-#define SHMALL (ULONG_MAX - (1L<<24))	 /* max shm system wide (pages) */
+#define SHMMAX (ULONG_MAX - (1UL << 24)) /* max shared seg size (bytes) */
+#define SHMALL (ULONG_MAX - (1UL << 24)) /* max shm system wide (pages) */
 #define SHMSEG SHMMNI			 /* max shared segs per process */
 
-
 /* Obsolete, used only for backwards compatibility and libc5 compiles */
 struct shmid_ds {
 	struct ipc_perm		shm_perm;	/* operation perms */
-- 
1.8.1.4




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/