[PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Manfred Spraul]

Hi all,

the increase of SHMMAX/SHMALL is now a 4 patch series.
I don't have ideas how to improve it further.

The change itself is trivial, the only problem are interger overflows.
The overflows are not new, but if we make huge values the default,
then the code should be free from overflows.

SHMMAX:

- shmmem_file_setup places a hard limit on the segment size:
  MAX_LFS_FILESIZE.

  On 32-bit, the limit is > 1 TB, i.e. 4 GB-1 byte segments are
  possible. Rounded up to full pages the actual allocated size
  is 0. --> must be fixed, patch 3

- shmat:
  - find_vma_intersection does not handle overflows properly.
    --> must be fixed, patch 1

  - the rest is fine, do_mmap_pgoff limits mappings to TASK_SIZE
    and checks for overflows (i.e.: map 2 GB, starting from
    addr=2.5GB fails).

SHMALL:
- after creating 8192 segments size (1L<<63)-1, shm_tot overflows and
  returns 0.  --> must be fixed, patch 2.

User space:
- Obviuosly, there could be overflows in user space. There is nothing
  we can do, only use values smaller than ULONG_MAX.
  I ended with "ULONG_MAX - 1L<<24":

  - TASK_SIZE cannot be used because it is the size of the current
    task. Could be 4G if it's a 32-bit task on a 64-bit kernel.

  - The maximum size is not standardized across archs:
    I found TASK_MAX_SIZE, TASK_SIZE_MAX and TASK_SIZE_64.

  - Just in case some arch revives a 4G/4G split, nearly
    ULONG_MAX is a valid segment size.

  - Using "0" as a magic value for infinity is even worse, because
    right now 0 means 0, i.e. fail all allocations.

Andrew: Could you add it into -akpm and move it towards linux-next?

--
	Manfred

[PATCH 1/4] ipc/shm.c: check for ulong overflows in shmat

[Manfred Spraul]

find_vma_intersection does not work as intended if addr+size overflows.
The patch adds a manual check before the call to find_vma_intersection.

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/shm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ipc/shm.c b/ipc/shm.c
index 7645961..382e2fb 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1160,6 +1160,9 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 	down_write(&current->mm->mmap_sem);
 	if (addr && !(shmflg & SHM_REMAP)) {
 		err = -EINVAL;
+		if (addr + size < addr)
+			goto invalid;
+
 		if (find_vma_intersection(current->mm, addr, addr + size))
 			goto invalid;
 		/*
-- 
1.9.0

[PATCH 2/4] ipc/shm.c: check for overflows of shm_tot

[Manfred Spraul]

shm_tot counts the total number of pages used by shm segments.

If SHMALL is ULONG_MAX (or nearly ULONG_MAX), then the number
can overflow.  Subsequent calls to shmctl(,SHM_INFO,) would return
wrong values for shm_tot.

The patch adds a detection for overflows.

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/shm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 382e2fb..2dfa3d6 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -493,7 +493,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 	if (size < SHMMIN || size > ns->shm_ctlmax)
 		return -EINVAL;
 
-	if (ns->shm_tot + numpages > ns->shm_ctlall)
+	if (ns->shm_tot + numpages < ns->shm_tot ||
+			ns->shm_tot + numpages > ns->shm_ctlall)
 		return -ENOSPC;
 
 	shp = ipc_rcu_alloc(sizeof(*shp));
-- 
1.9.0

[PATCH 3/4] ipc/shm.c: check for integer overflow during shmget.

[Manfred Spraul]

SHMMAX is the upper limit for the size of a shared memory segment,
counted in bytes. The actual allocation is that size, rounded up to
the next full page.
Add a check that prevents the creation of segments where the
rounded up size causes an integer overflow.

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/shm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ipc/shm.c b/ipc/shm.c
index 2dfa3d6..f000696 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -493,6 +493,9 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 	if (size < SHMMIN || size > ns->shm_ctlmax)
 		return -EINVAL;
 
+	if (numpages << PAGE_SHIFT < size)
+		return -ENOSPC;
+
 	if (ns->shm_tot + numpages < ns->shm_tot ||
 			ns->shm_tot + numpages > ns->shm_ctlall)
 		return -ENOSPC;
-- 
1.9.0

[PATCH 4/4] ipc/shm.c: Increase the defaults for SHMALL, SHMMAX.

[Manfred Spraul]

System V shared memory

a) can be abused to trigger out-of-memory conditions and the standard
   measures against out-of-memory do not work:

    - it is not possible to use setrlimit to limit the size of shm segments.

    - segments can exist without association with any processes, thus
      the oom-killer is unable to free that memory.

b) is typically used for shared information - today often multiple GB.
   (e.g. database shared buffers)

The current default is a maximum segment size of 32 MB and a maximum total
size of 8 GB. This is often too much for a) and not enough for b), which
means that lots of users must change the defaults.

This patch increases the default limits (nearly) to the maximum, which is
perfect for case b). The defaults are used after boot and as the initial
value for each new namespace.

Admins/distros that need a protection against a) should reduce the limits
and/or enable shm_rmid_forced.

Further notes:
- The patch only changes default, overrides behave as before:
        # sysctl kernel.shmall=33554432
  would recreate the previous limit for SHMMAX (for the current namespace).

- Disabling sysv shm allocation is possible with:
        # sysctl kernel.shmall=0
  (not a new feature, also per-namespace)

- The limits are intentionally set to a value slightly less than ULONG_MAX,
  to avoid triggering overflows in user space apps.
  [not unreasonable, see http://marc.info/?l=linux-mm&m=139638334330127]

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Reported-by: Davidlohr Bueso <davidlohr@hp.com>
Cc: mtk.manpages@gmail.com
---
 include/linux/shm.h      | 3 +--
 include/uapi/linux/shm.h | 8 +++-----
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/include/linux/shm.h b/include/linux/shm.h
index 1e2cd2e..57d7770 100644
--- a/include/linux/shm.h
+++ b/include/linux/shm.h
@@ -3,9 +3,8 @@
 
 #include <asm/page.h>
 #include <uapi/linux/shm.h>
-
-#define SHMALL (SHMMAX/PAGE_SIZE*(SHMMNI/16)) /* max shm system wide (pages) */
 #include <asm/shmparam.h>
+
 struct shmid_kernel /* private to the kernel */
 {	
 	struct kern_ipc_perm	shm_perm;
diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
index 78b6941..74e786d 100644
--- a/include/uapi/linux/shm.h
+++ b/include/uapi/linux/shm.h
@@ -9,15 +9,13 @@
 
 /*
  * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
- * be increased by sysctl
+ * be modified by sysctl.
  */
 
-#define SHMMAX 0x2000000		 /* max shared seg size (bytes) */
 #define SHMMIN 1			 /* min shared seg size (bytes) */
 #define SHMMNI 4096			 /* max num of segs system wide */
-#ifndef __KERNEL__
-#define SHMALL (SHMMAX/getpagesize()*(SHMMNI/16))
-#endif
+#define SHMMAX (ULONG_MAX - (1L<<24))	 /* max shared seg size (bytes) */
+#define SHMALL (ULONG_MAX - (1L<<24))	 /* max shm system wide (pages) */
 #define SHMSEG SHMMNI			 /* max shared segs per process */
 
 
-- 
1.9.0

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Davidlohr Bueso]

On Mon, 2014-04-21 at 16:26 +0200, Manfred Spraul wrote:
> Hi all,
> 
> the increase of SHMMAX/SHMALL is now a 4 patch series.
> I don't have ideas how to improve it further.

Manfred, is there any difference between this set and the one you sent a
couple of days ago?

> 
> The change itself is trivial, the only problem are interger overflows.
> The overflows are not new, but if we make huge values the default,
> then the code should be free from overflows.
> 
> SHMMAX:
> 
> - shmmem_file_setup places a hard limit on the segment size:
>   MAX_LFS_FILESIZE.
> 
>   On 32-bit, the limit is > 1 TB, i.e. 4 GB-1 byte segments are
>   possible. Rounded up to full pages the actual allocated size
>   is 0. --> must be fixed, patch 3
> 
> - shmat:
>   - find_vma_intersection does not handle overflows properly.
>     --> must be fixed, patch 1
> 
>   - the rest is fine, do_mmap_pgoff limits mappings to TASK_SIZE
>     and checks for overflows (i.e.: map 2 GB, starting from
>     addr=2.5GB fails).
> 
> SHMALL:
> - after creating 8192 segments size (1L<<63)-1, shm_tot overflows and
>   returns 0.  --> must be fixed, patch 2.
> 
> User space:
> - Obviuosly, there could be overflows in user space. There is nothing
>   we can do, only use values smaller than ULONG_MAX.
>   I ended with "ULONG_MAX - 1L<<24":
> 
>   - TASK_SIZE cannot be used because it is the size of the current
>     task. Could be 4G if it's a 32-bit task on a 64-bit kernel.
> 
>   - The maximum size is not standardized across archs:
>     I found TASK_MAX_SIZE, TASK_SIZE_MAX and TASK_SIZE_64.
> 
>   - Just in case some arch revives a 4G/4G split, nearly
>     ULONG_MAX is a valid segment size.
> 
>   - Using "0" as a magic value for infinity is even worse, because
>     right now 0 means 0, i.e. fail all allocations.

Sorry but I don't quite get this. Using 0 eliminates the need for all
these patches, no? I mean overflows have existed since forever, and
taking this route would naturally solve the problem. 0 allocations are a
no no anyways.

I do agree with the series iff we endup taking this 'increase the limit
size approach'. But I just don't see the need.

Thanks,
Davidlohr

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Manfred Spraul]

On 04/21/2014 07:25 PM, Davidlohr Bueso wrote:
> On Mon, 2014-04-21 at 16:26 +0200, Manfred Spraul wrote:
>> Hi all,
>>
>> the increase of SHMMAX/SHMALL is now a 4 patch series.
>> I don't have ideas how to improve it further.
> Manfred, is there any difference between this set and the one you sent a
> couple of days ago?
a) I updated the comments.
b) the initial set used TASK_SIZE, not I switch to ULONG_MAX-(1L<<24)

>>    - Using "0" as a magic value for infinity is even worse, because
>>      right now 0 means 0, i.e. fail all allocations.
> Sorry but I don't quite get this. Using 0 eliminates the need for all
> these patches, no? I mean overflows have existed since forever, and
> taking this route would naturally solve the problem. 0 allocations are a
> no no anyways.
No. The patches are required to handle e.g. shmget(,ULONG_MAX,):
Right now, shmget(,ULONG_MAX,) results in a 0-byte segment.

The risk of using 0 is that it reverses the current behavior:
Up to now,
     # sysctl kernel.shmall=0
disables allocations.
If we define 0 a infinity, then the same configuration would allow 
unlimited allocations.

--
     Manfred

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Davidlohr Bueso]

On Tue, 2014-04-22 at 06:23 +0200, Manfred Spraul wrote:
> On 04/21/2014 07:25 PM, Davidlohr Bueso wrote:
> > On Mon, 2014-04-21 at 16:26 +0200, Manfred Spraul wrote:
> >> Hi all,
> >>
> >> the increase of SHMMAX/SHMALL is now a 4 patch series.
> >> I don't have ideas how to improve it further.
> > Manfred, is there any difference between this set and the one you sent a
> > couple of days ago?
> a) I updated the comments.
> b) the initial set used TASK_SIZE, not I switch to ULONG_MAX-(1L<<24)
> 
> >>    - Using "0" as a magic value for infinity is even worse, because
> >>      right now 0 means 0, i.e. fail all allocations.
> > Sorry but I don't quite get this. Using 0 eliminates the need for all
> > these patches, no? I mean overflows have existed since forever, and
> > taking this route would naturally solve the problem. 0 allocations are a
> > no no anyways.
> No. The patches are required to handle e.g. shmget(,ULONG_MAX,):
> Right now, shmget(,ULONG_MAX,) results in a 0-byte segment.

Ok, I was mixing 'issues' then.

> The risk of using 0 is that it reverses the current behavior:
> Up to now,
>      # sysctl kernel.shmall=0
> disables allocations.
> If we define 0 a infinity, then the same configuration would allow 
> unlimited allocations.

Right, but as I mentioned, this also contradicts the fact that shmmin
cannot be 0. And again, I don't know who's correct here. Do any
standards mention this? I haven't found anything, and hard-codding
shmmin to 1 seems to be different among OSs, Linux choosing to do so.
This difference must also be commented in the manpage.

That said, I believe that violating this "feature" and forbidding
disabling shm would probably have a more severe penalty (security,
perhaps) for users who rely on this. So while I'm really annoyed that we
"cannot" use 0 because of this, I'm going to give up arguing. I believe
you approach is the safer way of going.

Thanks a lot for looking into this, Manfred.
Davidlohr

Re: [PATCH 1/4] ipc/shm.c: check for ulong overflows in shmat

[Davidlohr Bueso]

On Mon, 2014-04-21 at 16:26 +0200, Manfred Spraul wrote:
> find_vma_intersection does not work as intended if addr+size overflows.
> The patch adds a manual check before the call to find_vma_intersection.
> 
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>

Acked-by: Davidlohr Bueso <davidlohr@hp.com>

Re: [PATCH 2/4] ipc/shm.c: check for overflows of shm_tot

[Davidlohr Bueso]

On Mon, 2014-04-21 at 16:26 +0200, Manfred Spraul wrote:
> shm_tot counts the total number of pages used by shm segments.
> 
> If SHMALL is ULONG_MAX (or nearly ULONG_MAX), then the number
> can overflow.  Subsequent calls to shmctl(,SHM_INFO,) would return
> wrong values for shm_tot.
> 
> The patch adds a detection for overflows.
> 
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>

Acked-by: Davidlohr Bueso <davidlohr@hp.com>

Re: [PATCH 3/4] ipc/shm.c: check for integer overflow during shmget.

[Davidlohr Bueso]

On Mon, 2014-04-21 at 16:26 +0200, Manfred Spraul wrote:
> SHMMAX is the upper limit for the size of a shared memory segment,
> counted in bytes. The actual allocation is that size, rounded up to
> the next full page.
> Add a check that prevents the creation of segments where the
> rounded up size causes an integer overflow.
> 
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>

Acked-by: Davidlohr Bueso <davidlohr@hp.com>

Re: [PATCH 4/4] ipc/shm.c: Increase the defaults for SHMALL, SHMMAX.

[Davidlohr Bueso]

On Mon, 2014-04-21 at 16:26 +0200, Manfred Spraul wrote:
> System V shared memory
> 
> a) can be abused to trigger out-of-memory conditions and the standard
>    measures against out-of-memory do not work:
> 
>     - it is not possible to use setrlimit to limit the size of shm segments.
> 
>     - segments can exist without association with any processes, thus
>       the oom-killer is unable to free that memory.
> 
> b) is typically used for shared information - today often multiple GB.
>    (e.g. database shared buffers)
> 
> The current default is a maximum segment size of 32 MB and a maximum total
> size of 8 GB. This is often too much for a) and not enough for b), which
> means that lots of users must change the defaults.
> 
> This patch increases the default limits (nearly) to the maximum, which is
> perfect for case b). The defaults are used after boot and as the initial
> value for each new namespace.
> 
> Admins/distros that need a protection against a) should reduce the limits
> and/or enable shm_rmid_forced.
> 
> Further notes:
> - The patch only changes default, overrides behave as before:
>         # sysctl kernel.shmall=33554432
>   would recreate the previous limit for SHMMAX (for the current namespace).
> 
> - Disabling sysv shm allocation is possible with:
>         # sysctl kernel.shmall=0
>   (not a new feature, also per-namespace)
> 
> - The limits are intentionally set to a value slightly less than ULONG_MAX,
>   to avoid triggering overflows in user space apps.
>   [not unreasonable, see http://marc.info/?l=linux-mm&m=139638334330127]
> 
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> Reported-by: Davidlohr Bueso <davidlohr@hp.com>
> Cc: mtk.manpages@gmail.com

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>

With one comment below.

> ---
>  include/linux/shm.h      | 3 +--
>  include/uapi/linux/shm.h | 8 +++-----
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/shm.h b/include/linux/shm.h
> index 1e2cd2e..57d7770 100644
> --- a/include/linux/shm.h
> +++ b/include/linux/shm.h
> @@ -3,9 +3,8 @@
>  
>  #include <asm/page.h>
>  #include <uapi/linux/shm.h>
> -
> -#define SHMALL (SHMMAX/PAGE_SIZE*(SHMMNI/16)) /* max shm system wide (pages) */
>  #include <asm/shmparam.h>
> +
>  struct shmid_kernel /* private to the kernel */
>  {	
>  	struct kern_ipc_perm	shm_perm;
> diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
> index 78b6941..74e786d 100644
> --- a/include/uapi/linux/shm.h
> +++ b/include/uapi/linux/shm.h
> @@ -9,15 +9,13 @@
>  
>  /*
>   * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
> - * be increased by sysctl
> + * be modified by sysctl.
>   */
>  
> -#define SHMMAX 0x2000000		 /* max shared seg size (bytes) */
>  #define SHMMIN 1			 /* min shared seg size (bytes) */
>  #define SHMMNI 4096			 /* max num of segs system wide */
> -#ifndef __KERNEL__
> -#define SHMALL (SHMMAX/getpagesize()*(SHMMNI/16))
> -#endif
> +#define SHMMAX (ULONG_MAX - (1L<<24))	 /* max shared seg size (bytes) */
> +#define SHMALL (ULONG_MAX - (1L<<24))	 /* max shm system wide (pages) */

It's quite clear in the changelog, but could you please add a big fat
comment explaining this option, and that there's no point in enlarging
it. In fact if the user wants to make it bigger, we should display some
printk_once mentioning that this is the upper limit.

Thanks,
Davidlohr

Re: [PATCH 4/4] ipc/shm.c: Increase the defaults for SHMALL, SHMMAX.

[Davidlohr Bueso]

On Mon, 2014-04-21 at 16:26 +0200, Manfred Spraul wrote:
> System V shared memory
> 
> a) can be abused to trigger out-of-memory conditions and the standard
>    measures against out-of-memory do not work:
> 
>     - it is not possible to use setrlimit to limit the size of shm segments.
> 
>     - segments can exist without association with any processes, thus
>       the oom-killer is unable to free that memory.
> 
> b) is typically used for shared information - today often multiple GB.
>    (e.g. database shared buffers)
> 
> The current default is a maximum segment size of 32 MB and a maximum total
> size of 8 GB. This is often too much for a) and not enough for b), which
> means that lots of users must change the defaults.

Per Andrew's request, I think the following should go here from the
changelog of my patch:

Unix has historically required setting these limits for shared
memory, and Linux inherited such behavior. The consequence of this
is added complexity for users and administrators. One very common
example are Database setup/installation documents and scripts,
where users must manually calculate the values for these limits.
This also requires (some) knowledge of how the underlying memory
management works, thus causing, in many occasions, the limits to
just be flat out wrong. Disabling these limits sooner could have
saved companies a lot of time, headaches and money for support.
But it's never too late, simplify users life now.


> This patch increases the default limits (nearly) to the maximum, which is
> perfect for case b). The defaults are used after boot and as the initial
> value for each new namespace.
> 
> Admins/distros that need a protection against a) should reduce the limits
> and/or enable shm_rmid_forced.
> 
> Further notes:
> - The patch only changes default, overrides behave as before:
>         # sysctl kernel.shmall=33554432
>   would recreate the previous limit for SHMMAX (for the current namespace).
> 
> - Disabling sysv shm allocation is possible with:
>         # sysctl kernel.shmall=0
>   (not a new feature, also per-namespace)
> 
> - The limits are intentionally set to a value slightly less than ULONG_MAX,
>   to avoid triggering overflows in user space apps.
>   [not unreasonable, see http://marc.info/?l=linux-mm&m=139638334330127]
> 
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> Reported-by: Davidlohr Bueso <davidlohr@hp.com>
> Cc: mtk.manpages@gmail.com
> ---
>  include/linux/shm.h      | 3 +--
>  include/uapi/linux/shm.h | 8 +++-----
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/shm.h b/include/linux/shm.h
> index 1e2cd2e..57d7770 100644
> --- a/include/linux/shm.h
> +++ b/include/linux/shm.h
> @@ -3,9 +3,8 @@
>  
>  #include <asm/page.h>
>  #include <uapi/linux/shm.h>
> -
> -#define SHMALL (SHMMAX/PAGE_SIZE*(SHMMNI/16)) /* max shm system wide (pages) */
>  #include <asm/shmparam.h>
> +
>  struct shmid_kernel /* private to the kernel */
>  {	
>  	struct kern_ipc_perm	shm_perm;
> diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
> index 78b6941..74e786d 100644
> --- a/include/uapi/linux/shm.h
> +++ b/include/uapi/linux/shm.h
> @@ -9,15 +9,13 @@
>  
>  /*
>   * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
> - * be increased by sysctl
> + * be modified by sysctl.
>   */
>  
> -#define SHMMAX 0x2000000		 /* max shared seg size (bytes) */
>  #define SHMMIN 1			 /* min shared seg size (bytes) */
>  #define SHMMNI 4096			 /* max num of segs system wide */
> -#ifndef __KERNEL__
> -#define SHMALL (SHMMAX/getpagesize()*(SHMMNI/16))
> -#endif
> +#define SHMMAX (ULONG_MAX - (1L<<24))	 /* max shared seg size (bytes) */
> +#define SHMALL (ULONG_MAX - (1L<<24))	 /* max shm system wide (pages) */
>  #define SHMSEG SHMMNI			 /* max shared segs per process */
>  
> 

RE: [PATCH 1/4] ipc/shm.c: check for ulong overflows in shmat

[Motohiro Kosaki]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 488 bytes --]

> > find_vma_intersection does not work as intended if addr+size overflows.
> > The patch adds a manual check before the call to find_vma_intersection.
> >
> > Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> 
> Acked-by: Davidlohr Bueso <davidlohr@hp.com>

Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 2/4] ipc/shm.c: check for overflows of shm_tot

[Motohiro Kosaki]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 623 bytes --]

> > shm_tot counts the total number of pages used by shm segments.
> >
> > If SHMALL is ULONG_MAX (or nearly ULONG_MAX), then the number can
> > overflow.  Subsequent calls to shmctl(,SHM_INFO,) would return wrong
> > values for shm_tot.
> >
> > The patch adds a detection for overflows.
> >
> > Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> 
> Acked-by: Davidlohr Bueso <davidlohr@hp.com>

Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 3/4] ipc/shm.c: check for integer overflow during shmget.

[Motohiro Kosaki]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 615 bytes --]

> > SHMMAX is the upper limit for the size of a shared memory segment,
> > counted in bytes. The actual allocation is that size, rounded up to
> > the next full page.
> > Add a check that prevents the creation of segments where the rounded
> > up size causes an integer overflow.
> >
> > Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> 
> Acked-by: Davidlohr Bueso <davidlohr@hp.com>

Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 4/4] ipc/shm.c: Increase the defaults for SHMALL, SHMMAX.

[Motohiro Kosaki]

> -----Original Message-----
> From: Manfred Spraul [mailto:manfred@colorfullife.com]
> Sent: Monday, April 21, 2014 10:27 AM
> To: Davidlohr Bueso; Michael Kerrisk; Martin Schwidefsky
> Cc: LKML; Andrew Morton; KAMEZAWA Hiroyuki; Motohiro Kosaki JP; gthelen@google.com; aswin@hp.com; linux-mm@kvack.org;
> Manfred Spraul
> Subject: [PATCH 4/4] ipc/shm.c: Increase the defaults for SHMALL, SHMMAX.
> 
> System V shared memory
> 
> a) can be abused to trigger out-of-memory conditions and the standard
>    measures against out-of-memory do not work:
> 
>     - it is not possible to use setrlimit to limit the size of shm segments.
> 
>     - segments can exist without association with any processes, thus
>       the oom-killer is unable to free that memory.
> 
> b) is typically used for shared information - today often multiple GB.
>    (e.g. database shared buffers)
> 
> The current default is a maximum segment size of 32 MB and a maximum total size of 8 GB. This is often too much for a) and not
> enough for b), which means that lots of users must change the defaults.
> 
> This patch increases the default limits (nearly) to the maximum, which is perfect for case b). The defaults are used after boot and as
> the initial value for each new namespace.
> 
> Admins/distros that need a protection against a) should reduce the limits and/or enable shm_rmid_forced.
> 
> Further notes:
> - The patch only changes default, overrides behave as before:
>         # sysctl kernel.shmall=33554432
>   would recreate the previous limit for SHMMAX (for the current namespace).
> 
> - Disabling sysv shm allocation is possible with:
>         # sysctl kernel.shmall=0
>   (not a new feature, also per-namespace)
> 
> - The limits are intentionally set to a value slightly less than ULONG_MAX,
>   to avoid triggering overflows in user space apps.
>   [not unreasonable, see http://marc.info/?l=linux-mm&m=139638334330127]
> 
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> Reported-by: Davidlohr Bueso <davidlohr@hp.com>
> Cc: mtk.manpages@gmail.com

Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

[PATCH 5/4] ipc,shm: minor cleanups

[Davidlohr Bueso]

-  Breakup long function names/args.
-  Cleaup variable declaration.
-  s/current->mm/mm

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 ipc/shm.c | 40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index f000696..584d02e 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -480,15 +480,13 @@ static const struct vm_operations_struct shm_vm_ops = {
 static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 {
 	key_t key = params->key;
-	int shmflg = params->flg;
+	int id, error, shmflg = params->flg;
 	size_t size = params->u.size;
-	int error;
-	struct shmid_kernel *shp;
 	size_t numpages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	struct file *file;
 	char name[13];
-	int id;
 	vm_flags_t acctflag = 0;
+	struct shmid_kernel *shp;
+	struct file *file;
 
 	if (size < SHMMIN || size > ns->shm_ctlmax)
 		return -EINVAL;
@@ -681,7 +679,8 @@ copy_shmid_from_user(struct shmid64_ds *out, void __user *buf, int version)
 	}
 }
 
-static inline unsigned long copy_shminfo_to_user(void __user *buf, struct shminfo64 *in, int version)
+static inline unsigned long copy_shminfo_to_user(void __user *buf,
+						 struct shminfo64 *in, int version)
 {
 	switch (version) {
 	case IPC_64:
@@ -711,8 +710,8 @@ static inline unsigned long copy_shminfo_to_user(void __user *buf, struct shminf
  * Calculate and add used RSS and swap pages of a shm.
  * Called with shm_ids.rwsem held as a reader
  */
-static void shm_add_rss_swap(struct shmid_kernel *shp,
-	unsigned long *rss_add, unsigned long *swp_add)
+static void shm_add_rss_swap(struct shmid_kernel *shp, unsigned long *rss_add,
+			     unsigned long *swp_add)
 {
 	struct inode *inode;
 
@@ -739,7 +738,7 @@ static void shm_add_rss_swap(struct shmid_kernel *shp,
  * Called with shm_ids.rwsem held as a reader
  */
 static void shm_get_stat(struct ipc_namespace *ns, unsigned long *rss,
-		unsigned long *swp)
+			 unsigned long *swp)
 {
 	int next_id;
 	int total, in_use;
@@ -1047,21 +1046,16 @@ out_unlock1:
 long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 	      unsigned long shmlba)
 {
-	struct shmid_kernel *shp;
-	unsigned long addr;
-	unsigned long size;
+	unsigned long addr, size, flags, prot, populate = 0;
 	struct file *file;
-	int    err;
-	unsigned long flags;
-	unsigned long prot;
-	int acc_mode;
+	int acc_mode, err = -EINVAL;
 	struct ipc_namespace *ns;
 	struct shm_file_data *sfd;
+	struct shmid_kernel *shp;
 	struct path path;
 	fmode_t f_mode;
-	unsigned long populate = 0;
+	struct mm_struct *mm = current->mm;
 
-	err = -EINVAL;
 	if (shmid < 0)
 		goto out;
 	else if ((addr = (ulong)shmaddr)) {
@@ -1161,20 +1155,20 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 	if (err)
 		goto out_fput;
 
-	down_write(&current->mm->mmap_sem);
+	down_write(&mm->mmap_sem);
 	if (addr && !(shmflg & SHM_REMAP)) {
 		err = -EINVAL;
 		if (addr + size < addr)
 			goto invalid;
 
-		if (find_vma_intersection(current->mm, addr, addr + size))
+		if (find_vma_intersection(mm, addr, addr + size))
 			goto invalid;
 		/*
 		 * If shm segment goes below stack, make sure there is some
 		 * space left for the stack to grow (at least 4 pages).
 		 */
-		if (addr < current->mm->start_stack &&
-		    addr > current->mm->start_stack - size - PAGE_SIZE * 5)
+		if (addr < mm->start_stack &&
+		    addr > mm->start_stack - size - PAGE_SIZE * 5)
 			goto invalid;
 	}
 
@@ -1184,7 +1178,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
 	if (IS_ERR_VALUE(addr))
 		err = (long)addr;
 invalid:
-	up_write(&current->mm->mmap_sem);
+	up_write(&mm->mmap_sem);
 	if (populate)
 		mm_populate(addr, populate);
 
-- 
1.8.1.4

Re: [PATCH 1/4] ipc/shm.c: check for ulong overflows in shmat

[Michael Kerrisk (man-pages)]

On 04/21/2014 04:26 PM, Manfred Spraul wrote:
> find_vma_intersection does not work as intended if addr+size overflows.
> The patch adds a manual check before the call to find_vma_intersection.
> 
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> ---
>  ipc/shm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 7645961..382e2fb 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1160,6 +1160,9 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>  	down_write(&current->mm->mmap_sem);
>  	if (addr && !(shmflg & SHM_REMAP)) {
>  		err = -EINVAL;
> +		if (addr + size < addr)
> +			goto invalid;
> +
>  		if (find_vma_intersection(current->mm, addr, addr + size))
>  			goto invalid;
>  		/*
> 

Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 2/4] ipc/shm.c: check for overflows of shm_tot

[Michael Kerrisk (man-pages)]

On 04/21/2014 04:26 PM, Manfred Spraul wrote:
> shm_tot counts the total number of pages used by shm segments.
> 
> If SHMALL is ULONG_MAX (or nearly ULONG_MAX), then the number
> can overflow.  Subsequent calls to shmctl(,SHM_INFO,) would return
> wrong values for shm_tot.
> 
> The patch adds a detection for overflows.
> 
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> ---
>  ipc/shm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 382e2fb..2dfa3d6 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -493,7 +493,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
>  	if (size < SHMMIN || size > ns->shm_ctlmax)
>  		return -EINVAL;
>  
> -	if (ns->shm_tot + numpages > ns->shm_ctlall)
> +	if (ns->shm_tot + numpages < ns->shm_tot ||
> +			ns->shm_tot + numpages > ns->shm_ctlall)
>  		return -ENOSPC;
>  
>  	shp = ipc_rcu_alloc(sizeof(*shp));
> 

Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 3/4] ipc/shm.c: check for integer overflow during shmget.

[Michael Kerrisk (man-pages)]

On 04/21/2014 04:26 PM, Manfred Spraul wrote:
> SHMMAX is the upper limit for the size of a shared memory segment,
> counted in bytes. The actual allocation is that size, rounded up to
> the next full page.
> Add a check that prevents the creation of segments where the
> rounded up size causes an integer overflow.
> 
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> ---
>  ipc/shm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 2dfa3d6..f000696 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -493,6 +493,9 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
>  	if (size < SHMMIN || size > ns->shm_ctlmax)
>  		return -EINVAL;
>  
> +	if (numpages << PAGE_SHIFT < size)
> +		return -ENOSPC;
> +
>  	if (ns->shm_tot + numpages < ns->shm_tot ||
>  			ns->shm_tot + numpages > ns->shm_ctlall)
>  		return -ENOSPC;
> 

Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 4/4] ipc/shm.c: Increase the defaults for SHMALL, SHMMAX.

[Michael Kerrisk (man-pages)]

On 04/21/2014 04:26 PM, Manfred Spraul wrote:
> System V shared memory
> 
> a) can be abused to trigger out-of-memory conditions and the standard
>    measures against out-of-memory do not work:
> 
>     - it is not possible to use setrlimit to limit the size of shm segments.
> 
>     - segments can exist without association with any processes, thus
>       the oom-killer is unable to free that memory.
> 
> b) is typically used for shared information - today often multiple GB.
>    (e.g. database shared buffers)
> 
> The current default is a maximum segment size of 32 MB and a maximum total
> size of 8 GB. This is often too much for a) and not enough for b), which
> means that lots of users must change the defaults.
> 
> This patch increases the default limits (nearly) to the maximum, which is
> perfect for case b). The defaults are used after boot and as the initial
> value for each new namespace.
> 
> Admins/distros that need a protection against a) should reduce the limits
> and/or enable shm_rmid_forced.
> 
> Further notes:
> - The patch only changes default, overrides behave as before:
>         # sysctl kernel.shmall=33554432
>   would recreate the previous limit for SHMMAX (for the current namespace).
> 
> - Disabling sysv shm allocation is possible with:
>         # sysctl kernel.shmall=0
>   (not a new feature, also per-namespace)
> 
> - The limits are intentionally set to a value slightly less than ULONG_MAX,
>   to avoid triggering overflows in user space apps.
>   [not unreasonable, see http://marc.info/?l=linux-mm&m=139638334330127]
> 
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> Reported-by: Davidlohr Bueso <davidlohr@hp.com>
> Cc: mtk.manpages@gmail.com
> ---
>  include/linux/shm.h      | 3 +--
>  include/uapi/linux/shm.h | 8 +++-----
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/shm.h b/include/linux/shm.h
> index 1e2cd2e..57d7770 100644
> --- a/include/linux/shm.h
> +++ b/include/linux/shm.h
> @@ -3,9 +3,8 @@
>  
>  #include <asm/page.h>
>  #include <uapi/linux/shm.h>
> -
> -#define SHMALL (SHMMAX/PAGE_SIZE*(SHMMNI/16)) /* max shm system wide (pages) */
>  #include <asm/shmparam.h>
> +
>  struct shmid_kernel /* private to the kernel */
>  {	
>  	struct kern_ipc_perm	shm_perm;
> diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
> index 78b6941..74e786d 100644
> --- a/include/uapi/linux/shm.h
> +++ b/include/uapi/linux/shm.h
> @@ -9,15 +9,13 @@
>  
>  /*
>   * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
> - * be increased by sysctl
> + * be modified by sysctl.
>   */
>  
> -#define SHMMAX 0x2000000		 /* max shared seg size (bytes) */
>  #define SHMMIN 1			 /* min shared seg size (bytes) */
>  #define SHMMNI 4096			 /* max num of segs system wide */
> -#ifndef __KERNEL__
> -#define SHMALL (SHMMAX/getpagesize()*(SHMMNI/16))
> -#endif
> +#define SHMMAX (ULONG_MAX - (1L<<24))	 /* max shared seg size (bytes) */
> +#define SHMALL (ULONG_MAX - (1L<<24))	 /* max shm system wide (pages) */
>  #define SHMSEG SHMMNI			 /* max shared segs per process */

Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 5/4] ipc,shm: minor cleanups

[Michael Kerrisk (man-pages)]

On 04/23/2014 04:53 AM, Davidlohr Bueso wrote:
> -  Breakup long function names/args.
> -  Cleaup variable declaration.
> -  s/current->mm/mm
> 
> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> ---
>  ipc/shm.c | 40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index f000696..584d02e 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -480,15 +480,13 @@ static const struct vm_operations_struct shm_vm_ops = {
>  static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
>  {
>  	key_t key = params->key;
> -	int shmflg = params->flg;
> +	int id, error, shmflg = params->flg;

It's largely a matter of taste (and I may be in a minority), and I know
there's certainly precedent in the kernel code, but I don't much like the 
style of mixing variable declarations that have initializers, with other
unrelated declarations (e.g., variables without initializers). What is 
the gain? One less line of text? That's (IMO) more than offset by the 
small loss of readability.

Cheers,

Michael

>  	size_t size = params->u.size;
> -	int error;
> -	struct shmid_kernel *shp;
>  	size_t numpages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> -	struct file *file;
>  	char name[13];
> -	int id;
>  	vm_flags_t acctflag = 0;
> +	struct shmid_kernel *shp;
> +	struct file *file;
>  
>  	if (size < SHMMIN || size > ns->shm_ctlmax)
>  		return -EINVAL;
> @@ -681,7 +679,8 @@ copy_shmid_from_user(struct shmid64_ds *out, void __user *buf, int version)
>  	}
>  }
>  
> -static inline unsigned long copy_shminfo_to_user(void __user *buf, struct shminfo64 *in, int version)
> +static inline unsigned long copy_shminfo_to_user(void __user *buf,
> +						 struct shminfo64 *in, int version)
>  {
>  	switch (version) {
>  	case IPC_64:
> @@ -711,8 +710,8 @@ static inline unsigned long copy_shminfo_to_user(void __user *buf, struct shminf
>   * Calculate and add used RSS and swap pages of a shm.
>   * Called with shm_ids.rwsem held as a reader
>   */
> -static void shm_add_rss_swap(struct shmid_kernel *shp,
> -	unsigned long *rss_add, unsigned long *swp_add)
> +static void shm_add_rss_swap(struct shmid_kernel *shp, unsigned long *rss_add,
> +			     unsigned long *swp_add)
>  {
>  	struct inode *inode;
>  
> @@ -739,7 +738,7 @@ static void shm_add_rss_swap(struct shmid_kernel *shp,
>   * Called with shm_ids.rwsem held as a reader
>   */
>  static void shm_get_stat(struct ipc_namespace *ns, unsigned long *rss,
> -		unsigned long *swp)
> +			 unsigned long *swp)
>  {
>  	int next_id;
>  	int total, in_use;
> @@ -1047,21 +1046,16 @@ out_unlock1:
>  long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>  	      unsigned long shmlba)
>  {
> -	struct shmid_kernel *shp;
> -	unsigned long addr;
> -	unsigned long size;
> +	unsigned long addr, size, flags, prot, populate = 0;
>  	struct file *file;
> -	int    err;
> -	unsigned long flags;
> -	unsigned long prot;
> -	int acc_mode;
> +	int acc_mode, err = -EINVAL;
>  	struct ipc_namespace *ns;
>  	struct shm_file_data *sfd;
> +	struct shmid_kernel *shp;
>  	struct path path;
>  	fmode_t f_mode;
> -	unsigned long populate = 0;
> +	struct mm_struct *mm = current->mm;
>  
> -	err = -EINVAL;
>  	if (shmid < 0)
>  		goto out;
>  	else if ((addr = (ulong)shmaddr)) {
> @@ -1161,20 +1155,20 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>  	if (err)
>  		goto out_fput;
>  
> -	down_write(&current->mm->mmap_sem);
> +	down_write(&mm->mmap_sem);
>  	if (addr && !(shmflg & SHM_REMAP)) {
>  		err = -EINVAL;
>  		if (addr + size < addr)
>  			goto invalid;
>  
> -		if (find_vma_intersection(current->mm, addr, addr + size))
> +		if (find_vma_intersection(mm, addr, addr + size))
>  			goto invalid;
>  		/*
>  		 * If shm segment goes below stack, make sure there is some
>  		 * space left for the stack to grow (at least 4 pages).
>  		 */
> -		if (addr < current->mm->start_stack &&
> -		    addr > current->mm->start_stack - size - PAGE_SIZE * 5)
> +		if (addr < mm->start_stack &&
> +		    addr > mm->start_stack - size - PAGE_SIZE * 5)
>  			goto invalid;
>  	}
>  
> @@ -1184,7 +1178,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>  	if (IS_ERR_VALUE(addr))
>  		err = (long)addr;
>  invalid:
> -	up_write(&current->mm->mmap_sem);
> +	up_write(&mm->mmap_sem);
>  	if (populate)
>  		mm_populate(addr, populate);
>  
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 5/4] ipc,shm: minor cleanups

[Davidlohr Bueso]

On Wed, 2014-04-23 at 07:07 +0200, Michael Kerrisk (man-pages) wrote:
> On 04/23/2014 04:53 AM, Davidlohr Bueso wrote:
> > -  Breakup long function names/args.
> > -  Cleaup variable declaration.
> > -  s/current->mm/mm
> > 
> > Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> > ---
> >  ipc/shm.c | 40 +++++++++++++++++-----------------------
> >  1 file changed, 17 insertions(+), 23 deletions(-)
> > 
> > diff --git a/ipc/shm.c b/ipc/shm.c
> > index f000696..584d02e 100644
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -480,15 +480,13 @@ static const struct vm_operations_struct shm_vm_ops = {
> >  static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
> >  {
> >  	key_t key = params->key;
> > -	int shmflg = params->flg;
> > +	int id, error, shmflg = params->flg;
> 
> It's largely a matter of taste (and I may be in a minority), and I know
> there's certainly precedent in the kernel code, but I don't much like the 
> style of mixing variable declarations that have initializers, with other
> unrelated declarations (e.g., variables without initializers). What is 
> the gain? One less line of text? That's (IMO) more than offset by the 
> small loss of readability.

Yes, it's taste. And yes, your in the minority, at least in many core
kernel components and ipc.

Thanks,
Davidlohr

Re: [PATCH 5/4] ipc,shm: minor cleanups

[Michael Kerrisk (man-pages)]

On 04/23/2014 07:25 AM, Davidlohr Bueso wrote:
> On Wed, 2014-04-23 at 07:07 +0200, Michael Kerrisk (man-pages) wrote:
>> On 04/23/2014 04:53 AM, Davidlohr Bueso wrote:
>>> -  Breakup long function names/args.
>>> -  Cleaup variable declaration.
>>> -  s/current->mm/mm
>>>
>>> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
>>> ---
>>>  ipc/shm.c | 40 +++++++++++++++++-----------------------
>>>  1 file changed, 17 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/ipc/shm.c b/ipc/shm.c
>>> index f000696..584d02e 100644
>>> --- a/ipc/shm.c
>>> +++ b/ipc/shm.c
>>> @@ -480,15 +480,13 @@ static const struct vm_operations_struct shm_vm_ops = {
>>>  static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
>>>  {
>>>  	key_t key = params->key;
>>> -	int shmflg = params->flg;
>>> +	int id, error, shmflg = params->flg;
>>
>> It's largely a matter of taste (and I may be in a minority), and I know
>> there's certainly precedent in the kernel code, but I don't much like the 
>> style of mixing variable declarations that have initializers, with other
>> unrelated declarations (e.g., variables without initializers). What is 
>> the gain? One less line of text? That's (IMO) more than offset by the 
>> small loss of readability.
> 
> Yes, it's taste. And yes, your in the minority, at least in many core
> kernel components and ipc.

I figured so. Just giving the minority a small voice ;-).


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 5/4] ipc,shm: minor cleanups

[Manfred Spraul]

On 04/23/2014 04:53 AM, Davidlohr Bueso wrote:
> -  Breakup long function names/args.
> -  Cleaup variable declaration.
s/Cleaup/Cleanup/
> -  s/current->mm/mm
>
> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>

> @@ -681,7 +679,8 @@ copy_shmid_from_user(struct shmid64_ds *out, void __user *buf, int version)
>   	}
>   }
>   
> -static inline unsigned long copy_shminfo_to_user(void __user *buf, struct shminfo64 *in, int version)
> +static inline unsigned long copy_shminfo_to_user(void __user *buf,
> +						 struct shminfo64 *in, int version)
Checkpatch still complains - does removing one tab help?

--
     Manfred

Re: [PATCH 5/4] ipc,shm: minor cleanups

[Andrew Morton]

On Tue, 22 Apr 2014 22:25:45 -0700 Davidlohr Bueso <davidlohr@hp.com> wrote:

> On Wed, 2014-04-23 at 07:07 +0200, Michael Kerrisk (man-pages) wrote:
> > On 04/23/2014 04:53 AM, Davidlohr Bueso wrote:
> > > -  Breakup long function names/args.
> > > -  Cleaup variable declaration.
> > > -  s/current->mm/mm
> > > 
> > > Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> > > ---
> > >  ipc/shm.c | 40 +++++++++++++++++-----------------------
> > >  1 file changed, 17 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/ipc/shm.c b/ipc/shm.c
> > > index f000696..584d02e 100644
> > > --- a/ipc/shm.c
> > > +++ b/ipc/shm.c
> > > @@ -480,15 +480,13 @@ static const struct vm_operations_struct shm_vm_ops = {
> > >  static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
> > >  {
> > >  	key_t key = params->key;
> > > -	int shmflg = params->flg;
> > > +	int id, error, shmflg = params->flg;
> > 
> > It's largely a matter of taste (and I may be in a minority), and I know
> > there's certainly precedent in the kernel code, but I don't much like the 
> > style of mixing variable declarations that have initializers, with other
> > unrelated declarations (e.g., variables without initializers). What is 
> > the gain? One less line of text? That's (IMO) more than offset by the 
> > small loss of readability.
> 
> Yes, it's taste. And yes, your in the minority, at least in many core
> kernel components and ipc.

I'm with Michael.

- Putting multiple definitions on the same line (whether or not they
  are initialized there) makes it impossible to add little comments
  documenting them.  And we need more little comments documenting
  locals.

- Having multiple definitions on the same line is maddening when the
  time comes to resolve patch conflicts.  And it increases the
  likelihood of conflicts in the first place.

- It makes it much harder to *find* a definition.

Re: [PATCH 5/4] ipc,shm: minor cleanups

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2300 bytes --]

On Wed, 23 Apr 2014 15:27:55 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue, 22 Apr 2014 22:25:45 -0700 Davidlohr Bueso <davidlohr@hp.com> wrote:
> 
> > On Wed, 2014-04-23 at 07:07 +0200, Michael Kerrisk (man-pages) wrote:
> > > On 04/23/2014 04:53 AM, Davidlohr Bueso wrote:
> > > > -  Breakup long function names/args.
> > > > -  Cleaup variable declaration.
> > > > -  s/current->mm/mm
> > > > 
> > > > Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> > > > ---
> > > >  ipc/shm.c | 40 +++++++++++++++++-----------------------
> > > >  1 file changed, 17 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/ipc/shm.c b/ipc/shm.c
> > > > index f000696..584d02e 100644
> > > > --- a/ipc/shm.c
> > > > +++ b/ipc/shm.c
> > > > @@ -480,15 +480,13 @@ static const struct vm_operations_struct shm_vm_ops = {
> > > >  static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
> > > >  {
> > > >  	key_t key = params->key;
> > > > -	int shmflg = params->flg;
> > > > +	int id, error, shmflg = params->flg;
> > > 
> > > It's largely a matter of taste (and I may be in a minority), and I know
> > > there's certainly precedent in the kernel code, but I don't much like the 
> > > style of mixing variable declarations that have initializers, with other
> > > unrelated declarations (e.g., variables without initializers). What is 
> > > the gain? One less line of text? That's (IMO) more than offset by the 
> > > small loss of readability.
> > 
> > Yes, it's taste. And yes, your in the minority, at least in many core
> > kernel components and ipc.
> 
> I'm with Michael.
> 
> - Putting multiple definitions on the same line (whether or not they
>   are initialized there) makes it impossible to add little comments
>   documenting them.  And we need more little comments documenting
>   locals.
> 
> - Having multiple definitions on the same line is maddening when the
>   time comes to resolve patch conflicts.  And it increases the
>   likelihood of conflicts in the first place.
> 
> - It makes it much harder to *find* a definition.

And it changes a line that has nothing to do with the patch.

Sometimes the minority are right :-)
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/4] ipc,shm: minor cleanups

[Michael Kerrisk (man-pages)]

On 04/23/2014 07:25 AM, Davidlohr Bueso wrote:
> On Wed, 2014-04-23 at 07:07 +0200, Michael Kerrisk (man-pages) wrote:
>> On 04/23/2014 04:53 AM, Davidlohr Bueso wrote:
>>> -  Breakup long function names/args.
>>> -  Cleaup variable declaration.
>>> -  s/current->mm/mm
>>>
>>> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
>>> ---
>>>  ipc/shm.c | 40 +++++++++++++++++-----------------------
>>>  1 file changed, 17 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/ipc/shm.c b/ipc/shm.c
>>> index f000696..584d02e 100644
>>> --- a/ipc/shm.c
>>> +++ b/ipc/shm.c
>>> @@ -480,15 +480,13 @@ static const struct vm_operations_struct shm_vm_ops = {
>>>  static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
>>>  {
>>>  	key_t key = params->key;
>>> -	int shmflg = params->flg;
>>> +	int id, error, shmflg = params->flg;
>>
>> It's largely a matter of taste (and I may be in a minority), and I know
>> there's certainly precedent in the kernel code, but I don't much like the 
>> style of mixing variable declarations that have initializers, with other
>> unrelated declarations (e.g., variables without initializers). What is 
>> the gain? One less line of text? That's (IMO) more than offset by the 
>> small loss of readability.
> 
> Yes, it's taste. And yes, your in the minority, at least in many core
> kernel components and ipc.

Davidlohr,

So, noting that the minority is less small than we thought, I'll just
add this: I'd have appreciated it if your reply had been less 
dismissive, and you'd actually responded to my concrete point about 
loss of readability.

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 5/4] ipc,shm: minor cleanups

[Davidlohr Bueso]

On Thu, 2014-04-24 at 07:18 +0200, Michael Kerrisk (man-pages) wrote:
> On 04/23/2014 07:25 AM, Davidlohr Bueso wrote:
> > On Wed, 2014-04-23 at 07:07 +0200, Michael Kerrisk (man-pages) wrote:
> >> On 04/23/2014 04:53 AM, Davidlohr Bueso wrote:
> >>> -  Breakup long function names/args.
> >>> -  Cleaup variable declaration.
> >>> -  s/current->mm/mm
> >>>
> >>> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> >>> ---
> >>>  ipc/shm.c | 40 +++++++++++++++++-----------------------
> >>>  1 file changed, 17 insertions(+), 23 deletions(-)
> >>>
> >>> diff --git a/ipc/shm.c b/ipc/shm.c
> >>> index f000696..584d02e 100644
> >>> --- a/ipc/shm.c
> >>> +++ b/ipc/shm.c
> >>> @@ -480,15 +480,13 @@ static const struct vm_operations_struct shm_vm_ops = {
> >>>  static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
> >>>  {
> >>>  	key_t key = params->key;
> >>> -	int shmflg = params->flg;
> >>> +	int id, error, shmflg = params->flg;
> >>
> >> It's largely a matter of taste (and I may be in a minority), and I know
> >> there's certainly precedent in the kernel code, but I don't much like the 
> >> style of mixing variable declarations that have initializers, with other
> >> unrelated declarations (e.g., variables without initializers). What is 
> >> the gain? One less line of text? That's (IMO) more than offset by the 
> >> small loss of readability.
> > 
> > Yes, it's taste. And yes, your in the minority, at least in many core
> > kernel components and ipc.
> 
> Davidlohr,
> 
> So, noting that the minority is less small than we thought, I'll just
> add this: I'd have appreciated it if your reply had been less 
> dismissive, and you'd actually responded to my concrete point about 
> loss of readability.

Apologies, I didn't mean to sound dismissive. It's just that I don't
like arguing over this kind of things. The idea of the cleanups wasn't
"lets remove LoC", but more "lets make the style suck less" -- and
believe me, ipc code is pretty darn ugly wrt. Over the last few months
we've improved it some, but still so much horror. The changes I make are
aligned with the general coding style we have in the rest of the kernel,
but yes, ultimately it comes down to taste.

Anyway, I am in favor of single line declarations with initializers
which are *meaningful*. The variables I moved around are not.

Thanks,
Davidlohr

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Michael Kerrisk (man-pages)]

Hi Manfred,

On Mon, Apr 21, 2014 at 4:26 PM, Manfred Spraul
<manfred@colorfullife.com> wrote:
> Hi all,
>
> the increase of SHMMAX/SHMALL is now a 4 patch series.
> I don't have ideas how to improve it further.

On the assumption that your patches are heading to mainline, could you
send me a man-pages patch for the changes?

Thanks,

Michael


> The change itself is trivial, the only problem are interger overflows.
> The overflows are not new, but if we make huge values the default,
> then the code should be free from overflows.
>
> SHMMAX:
>
> - shmmem_file_setup places a hard limit on the segment size:
>   MAX_LFS_FILESIZE.
>
>   On 32-bit, the limit is > 1 TB, i.e. 4 GB-1 byte segments are
>   possible. Rounded up to full pages the actual allocated size
>   is 0. --> must be fixed, patch 3
>
> - shmat:
>   - find_vma_intersection does not handle overflows properly.
>     --> must be fixed, patch 1
>
>   - the rest is fine, do_mmap_pgoff limits mappings to TASK_SIZE
>     and checks for overflows (i.e.: map 2 GB, starting from
>     addr=2.5GB fails).
>
> SHMALL:
> - after creating 8192 segments size (1L<<63)-1, shm_tot overflows and
>   returns 0.  --> must be fixed, patch 2.
>
> User space:
> - Obviuosly, there could be overflows in user space. There is nothing
>   we can do, only use values smaller than ULONG_MAX.
>   I ended with "ULONG_MAX - 1L<<24":
>
>   - TASK_SIZE cannot be used because it is the size of the current
>     task. Could be 4G if it's a 32-bit task on a 64-bit kernel.
>
>   - The maximum size is not standardized across archs:
>     I found TASK_MAX_SIZE, TASK_SIZE_MAX and TASK_SIZE_64.
>
>   - Just in case some arch revives a 4G/4G split, nearly
>     ULONG_MAX is a valid segment size.
>
>   - Using "0" as a magic value for infinity is even worse, because
>     right now 0 means 0, i.e. fail all allocations.
>
> Andrew: Could you add it into -akpm and move it towards linux-next?
>
> --
>         Manfred



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Davidlohr Bueso]

On Fri, 2014-05-02 at 15:16 +0200, Michael Kerrisk (man-pages) wrote:
> Hi Manfred,
> 
> On Mon, Apr 21, 2014 at 4:26 PM, Manfred Spraul
> <manfred@colorfullife.com> wrote:
> > Hi all,
> >
> > the increase of SHMMAX/SHMALL is now a 4 patch series.
> > I don't have ideas how to improve it further.
> 
> On the assumption that your patches are heading to mainline, could you
> send me a man-pages patch for the changes?

Btw, I think that the code could still use some love wrt documentation.
Andrew, please consider this for -next if folks agree. Thanks.

8<----------------------------------------------------------

From: Davidlohr Bueso <davidlohr@hp.com>
Subject: [PATCH] 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.

Also use UL as we're dealing with it anyways.

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

diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
index 74e786d..e37fb08 100644
--- a/include/uapi/linux/shm.h
+++ b/include/uapi/linux/shm.h
@@ -8,17 +8,19 @@
 #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 variables. It is therefore not advised to
+ * make them any larger. This is 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

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Michael Kerrisk (man-pages)]

Hi Davidlohr,

On Tue, May 6, 2014 at 10:06 PM, Davidlohr Bueso <davidlohr@hp.com> wrote:
> On Fri, 2014-05-02 at 15:16 +0200, Michael Kerrisk (man-pages) wrote:
>> Hi Manfred,
>>
>> On Mon, Apr 21, 2014 at 4:26 PM, Manfred Spraul
>> <manfred@colorfullife.com> wrote:
>> > Hi all,
>> >
>> > the increase of SHMMAX/SHMALL is now a 4 patch series.
>> > I don't have ideas how to improve it further.
>>
>> On the assumption that your patches are heading to mainline, could you
>> send me a man-pages patch for the changes?
>
> Btw, I think that the code could still use some love wrt documentation.

(Agreed.)

> Andrew, please consider this for -next if folks agree. Thanks.
>
> 8<----------------------------------------------------------
>
> From: Davidlohr Bueso <davidlohr@hp.com>
> Subject: [PATCH] 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.
>
> Also use UL as we're dealing with it anyways.
>
> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> ---
>  include/uapi/linux/shm.h | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
> index 74e786d..e37fb08 100644
> --- a/include/uapi/linux/shm.h
> +++ b/include/uapi/linux/shm.h
> @@ -8,17 +8,19 @@
>  #endif
>
>  /*
> - * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can

Something is wrong in the line above (missing word(s)?) ("are upper
limits are defaults")

> - * 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 variables. It is therefore not advised to

Something is missing in that last line.

> + * make them any larger. This is suitable for both 32 and 64-bit systems.

"This" is not so clear. I suggest replacing with an actual noun.

>   */
> -
>  #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 */

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Davidlohr Bueso]

On Tue, 2014-05-06 at 13:06 -0700, Davidlohr Bueso wrote:
> On Fri, 2014-05-02 at 15:16 +0200, Michael Kerrisk (man-pages) wrote:
> > Hi Manfred,
> > 
> > On Mon, Apr 21, 2014 at 4:26 PM, Manfred Spraul
> > <manfred@colorfullife.com> wrote:
> > > Hi all,
> > >
> > > the increase of SHMMAX/SHMALL is now a 4 patch series.
> > > I don't have ideas how to improve it further.
> > 
> > On the assumption that your patches are heading to mainline, could you
> > send me a man-pages patch for the changes?
> 
> Btw, I think that the code could still use some love wrt documentation.
> Andrew, please consider this for -next if folks agree. Thanks.
> 
> 8<----------------------------------------------------------
> 
> From: Davidlohr Bueso <davidlohr@hp.com>
> Subject: [PATCH] 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.
> 
> Also use UL as we're dealing with it anyways.
> 
> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> ---
>  include/uapi/linux/shm.h | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
> index 74e786d..e37fb08 100644
> --- a/include/uapi/linux/shm.h
> +++ b/include/uapi/linux/shm.h
> @@ -8,17 +8,19 @@
>  #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 variables. It is therefore not advised to
                              ^^^^^^^^^^ should be 'further', sorry.

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Davidlohr Bueso]

On Tue, 2014-05-06 at 22:40 +0200, Michael Kerrisk (man-pages) wrote:
> Hi Davidlohr,
> 
> On Tue, May 6, 2014 at 10:06 PM, Davidlohr Bueso <davidlohr@hp.com> wrote:
> > On Fri, 2014-05-02 at 15:16 +0200, Michael Kerrisk (man-pages) wrote:
> >> Hi Manfred,
> >>
> >> On Mon, Apr 21, 2014 at 4:26 PM, Manfred Spraul
> >> <manfred@colorfullife.com> wrote:
> >> > Hi all,
> >> >
> >> > the increase of SHMMAX/SHMALL is now a 4 patch series.
> >> > I don't have ideas how to improve it further.
> >>
> >> On the assumption that your patches are heading to mainline, could you
> >> send me a man-pages patch for the changes?
> >
> > Btw, I think that the code could still use some love wrt documentation.
> 
> (Agreed.)
> 
> > Andrew, please consider this for -next if folks agree. Thanks.
> >
> > 8<----------------------------------------------------------
> >
> > From: Davidlohr Bueso <davidlohr@hp.com>
> > Subject: [PATCH] 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.
> >
> > Also use UL as we're dealing with it anyways.
> >
> > Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> > ---
> >  include/uapi/linux/shm.h | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
> > index 74e786d..e37fb08 100644
> > --- a/include/uapi/linux/shm.h
> > +++ b/include/uapi/linux/shm.h
> > @@ -8,17 +8,19 @@
> >  #endif
> >
> >  /*
> > - * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
> 
> Something is wrong in the line above (missing word(s)?) ("are upper
> limits are defaults")
> 
> > - * 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 variables. It is therefore not advised to
> 
> Something is missing in that last line.
> 
> > + * make them any larger. This is suitable for both 32 and 64-bit systems.
> 
> "This" is not so clear. I suggest replacing with an actual noun.

Good point. Perhaps 'These values are ...' would do instead. 

Thanks,
Davidlohr

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Michael Kerrisk (man-pages)]

On 05/07/2014 12:08 AM, Davidlohr Bueso wrote:
> On Tue, 2014-05-06 at 22:40 +0200, Michael Kerrisk (man-pages) wrote:
>> Hi Davidlohr,
>>
>> On Tue, May 6, 2014 at 10:06 PM, Davidlohr Bueso <davidlohr@hp.com> wrote:
>>> On Fri, 2014-05-02 at 15:16 +0200, Michael Kerrisk (man-pages) wrote:
>>>> Hi Manfred,
>>>>
>>>> On Mon, Apr 21, 2014 at 4:26 PM, Manfred Spraul
>>>> <manfred@colorfullife.com> wrote:
>>>>> Hi all,
>>>>>
>>>>> the increase of SHMMAX/SHMALL is now a 4 patch series.
>>>>> I don't have ideas how to improve it further.
>>>>
>>>> On the assumption that your patches are heading to mainline, could you
>>>> send me a man-pages patch for the changes?
>>>
>>> Btw, I think that the code could still use some love wrt documentation.
>>
>> (Agreed.)
>>
>>> Andrew, please consider this for -next if folks agree. Thanks.
>>>
>>> 8<----------------------------------------------------------
>>>
>>> From: Davidlohr Bueso <davidlohr@hp.com>
>>> Subject: [PATCH] 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.
>>>
>>> Also use UL as we're dealing with it anyways.
>>>
>>> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
>>> ---
>>>  include/uapi/linux/shm.h | 14 ++++++++------
>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
>>> index 74e786d..e37fb08 100644
>>> --- a/include/uapi/linux/shm.h
>>> +++ b/include/uapi/linux/shm.h
>>> @@ -8,17 +8,19 @@
>>>  #endif
>>>
>>>  /*
>>> - * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
>>
>> Something is wrong in the line above (missing word(s)?) ("are upper
>> limits are defaults")
>>
>>> - * 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 variables. It is therefore not advised to
>>
>> Something is missing in that last line.
>>
>>> + * make them any larger. This is suitable for both 32 and 64-bit systems.
>>
>> "This" is not so clear. I suggest replacing with an actual noun.
> 
> Good point. Perhaps 'These values are ...' would do instead. 

That's better.

Did you miss the first point I raised above?

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Davidlohr Bueso]

On Wed, 2014-05-07 at 07:27 +0200, Michael Kerrisk (man-pages) wrote:
> On 05/07/2014 12:08 AM, Davidlohr Bueso wrote:
> > On Tue, 2014-05-06 at 22:40 +0200, Michael Kerrisk (man-pages) wrote:
> >> Hi Davidlohr,
> >>
> >> On Tue, May 6, 2014 at 10:06 PM, Davidlohr Bueso <davidlohr@hp.com> wrote:
> >>> On Fri, 2014-05-02 at 15:16 +0200, Michael Kerrisk (man-pages) wrote:
> >>>> Hi Manfred,
> >>>>
> >>>> On Mon, Apr 21, 2014 at 4:26 PM, Manfred Spraul
> >>>> <manfred@colorfullife.com> wrote:
> >>>>> Hi all,
> >>>>>
> >>>>> the increase of SHMMAX/SHMALL is now a 4 patch series.
> >>>>> I don't have ideas how to improve it further.
> >>>>
> >>>> On the assumption that your patches are heading to mainline, could you
> >>>> send me a man-pages patch for the changes?
> >>>
> >>> Btw, I think that the code could still use some love wrt documentation.
> >>
> >> (Agreed.)
> >>
> >>> Andrew, please consider this for -next if folks agree. Thanks.
> >>>
> >>> 8<----------------------------------------------------------
> >>>
> >>> From: Davidlohr Bueso <davidlohr@hp.com>
> >>> Subject: [PATCH] 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.
> >>>
> >>> Also use UL as we're dealing with it anyways.
> >>>
> >>> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> >>> ---
> >>>  include/uapi/linux/shm.h | 14 ++++++++------
> >>>  1 file changed, 8 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
> >>> index 74e786d..e37fb08 100644
> >>> --- a/include/uapi/linux/shm.h
> >>> +++ b/include/uapi/linux/shm.h
> >>> @@ -8,17 +8,19 @@
> >>>  #endif
> >>>
> >>>  /*
> >>> - * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
> >>
> >> Something is wrong in the line above (missing word(s)?) ("are upper
> >> limits are defaults")
> >>
> >>> - * 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 variables. It is therefore not advised to
> >>
> >> Something is missing in that last line.
> >>
> >>> + * make them any larger. This is suitable for both 32 and 64-bit systems.
> >>
> >> "This" is not so clear. I suggest replacing with an actual noun.
> > 
> > Good point. Perhaps 'These values are ...' would do instead. 
> 
> That's better.
> 
> Did you miss the first point I raised above?

No, actually our emails crossed paths and I had sent a suggestion before
I replied to yours: https://lkml.org/lkml/2014/5/6/613

Thanks.
Davidlohr

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

[Davidlohr Bueso]

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

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 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.
  */
-
 #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

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

[Michael Kerrisk (man-pages)]

On Wed, May 7, 2014 at 9:17 PM, Davidlohr Bueso <davidlohr@hp.com> wrote:
> This is useful in the future and allows users to
> better understand the reasoning behind the changes.
>
> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> ---
>  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.

?

Cheers,

Michael


>   */
> -
>  #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
>
>
>



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

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

[Davidlohr Bueso]

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@hp.com> wrote:
> > This is useful in the future and allows users to
> > better understand the reasoning behind the changes.
> >
> > Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> > ---
> >  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?

Thanks,
Davidlohr

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

[Michael Kerrisk (man-pages)]

Hi Davidlohr,

On Sun, May 11, 2014 at 10:46 PM, Davidlohr Bueso <davidlohr@hp.com> 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@hp.com> wrote:
>> > This is useful in the future and allows users to
>> > better understand the reasoning behind the changes.
>> >
>> > Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
>> > ---
>> >  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.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

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@hp.com> 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@hp.com> wrote:
> >> > This is useful in the future and allows users to
> >> > better understand the reasoning behind the changes.
> >> >
> >> > Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> >> > ---
> >> >  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@hp.com>
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@hp.com>
---
 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

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

[Michael Kerrisk (man-pages)]

On Tue, May 13, 2014 at 3:35 AM, Davidlohr Bueso <davidlohr@hp.com> wrote:
> On Mon, 2014-05-12 at 09:44 +0200, Michael Kerrisk (man-pages) wrote:
[...]
>> 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.

Thanks.

Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>

Cheers,'

Michael

> 8<------------------------------------------
> From: Davidlohr Bueso <davidlohr@hp.com>
> 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@hp.com>
> ---
>  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
>
>
>
>



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Davidlohr Bueso]

On Fri, 2014-05-02 at 15:16 +0200, Michael Kerrisk (man-pages) wrote:
> Hi Manfred,
> 
> On Mon, Apr 21, 2014 at 4:26 PM, Manfred Spraul
> <manfred@colorfullife.com> wrote:
> > Hi all,
> >
> > the increase of SHMMAX/SHMALL is now a 4 patch series.
> > I don't have ideas how to improve it further.
> 
> On the assumption that your patches are heading to mainline, could you
> send me a man-pages patch for the changes?

It seems we're still behind here and the 3.16 merge window is already
opened. Please consider this, and again feel free to add/modify as
necessary. I think adding a note as below is enough and was hesitant to
add a lot of details... Thanks.

8<--------------------------------------------------
From: Davidlohr Bueso <davidlohr@hp.com>
Subject: [PATCH] shmget.2: document new limits for shmmax/shmall

These limits have been recently enlarged and
modifying them is no longer really necessary.
Update the manpage.

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 man2/shmget.2 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/man2/shmget.2 b/man2/shmget.2
index f781048..77764ea 100644
--- a/man2/shmget.2
+++ b/man2/shmget.2
@@ -299,6 +299,11 @@ with 8kB page size, it yields 2^20 (1048576).
 
 On Linux, this limit can be read and modified via
 .IR /proc/sys/kernel/shmall .
+As of Linux 3.16, the default value for this limit is increased to
+.B ULONG_MAX - 2^24
+pages, which is as large as it can be without helping userspace overflow
+the values. Modifying this limit is therefore discouraged. This is suitable
+for both 32 and 64-bit systems.
 .TP
 .B SHMMAX
 Maximum size in bytes for a shared memory segment.
@@ -306,6 +311,12 @@ Since Linux 2.2, the default value of this limit is 0x2000000 (32MB).
 
 On Linux, this limit can be read and modified via
 .IR /proc/sys/kernel/shmmax .
+As of Linux 3.16, the default value for this limit is increased from 32Mb
+to
+.B ULONG_MAX - 2^24
+bytes, which is as large as it can be without helping userspace overflow
+the values. Modifying this limit is therefore discouraged. This is suitable
+for both 32 and 64-bit systems.
 .TP
 .B SHMMIN
 Minimum size in bytes for a shared memory segment: implementation
-- 
1.8.1.4

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Michael Kerrisk (man-pages)]

On 06/03/2014 09:26 PM, Davidlohr Bueso wrote:
> On Fri, 2014-05-02 at 15:16 +0200, Michael Kerrisk (man-pages) wrote:
>> Hi Manfred,
>>
>> On Mon, Apr 21, 2014 at 4:26 PM, Manfred Spraul
>> <manfred@colorfullife.com> wrote:
>>> Hi all,
>>>
>>> the increase of SHMMAX/SHMALL is now a 4 patch series.
>>> I don't have ideas how to improve it further.
>>
>> On the assumption that your patches are heading to mainline, could you
>> send me a man-pages patch for the changes?
> 
> It seems we're still behind here and the 3.16 merge window is already
> opened. Please consider this, and again feel free to add/modify as
> necessary. I think adding a note as below is enough and was hesitant to
> add a lot of details... Thanks.
> 
> 8<--------------------------------------------------
> From: Davidlohr Bueso <davidlohr@hp.com>
> Subject: [PATCH] shmget.2: document new limits for shmmax/shmall
> 
> These limits have been recently enlarged and
> modifying them is no longer really necessary.
> Update the manpage.
> 
> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> ---
>  man2/shmget.2 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/man2/shmget.2 b/man2/shmget.2
> index f781048..77764ea 100644
> --- a/man2/shmget.2
> +++ b/man2/shmget.2
> @@ -299,6 +299,11 @@ with 8kB page size, it yields 2^20 (1048576).
>  
>  On Linux, this limit can be read and modified via
>  .IR /proc/sys/kernel/shmall .
> +As of Linux 3.16, the default value for this limit is increased to
> +.B ULONG_MAX - 2^24
> +pages, which is as large as it can be without helping userspace overflow
> +the values. Modifying this limit is therefore discouraged. This is suitable
> +for both 32 and 64-bit systems.
>  .TP
>  .B SHMMAX
>  Maximum size in bytes for a shared memory segment.
> @@ -306,6 +311,12 @@ Since Linux 2.2, the default value of this limit is 0x2000000 (32MB).
>  
>  On Linux, this limit can be read and modified via
>  .IR /proc/sys/kernel/shmmax .
> +As of Linux 3.16, the default value for this limit is increased from 32Mb
> +to
> +.B ULONG_MAX - 2^24
> +bytes, which is as large as it can be without helping userspace overflow
> +the values. Modifying this limit is therefore discouraged. This is suitable
> +for both 32 and 64-bit systems.
>  .TP
>  .B SHMMIN
>  Minimum size in bytes for a shared memory segment: implementation

David,

I applied various pieces from your patch on top of material
that I already had, so that now we have the text below describing
these limits.  Comments/suggestions/improvements from all welcome.

Cheers,

Michael

       SHMALL System-wide limit on the number of pages of shared memory.

              On  Linux,  this  limit  can  be  read  and  modified  via
              /proc/sys/kernel/shmall.  Since Linux  3.16,  the  default
              value for this limit is:

                  ULONG_MAX - 2^24

              The  effect  of  this  value  (which  is suitable for both
              32-bit and 64-bit systems) is to impose no  limitation  on
              allocations.   This value, rather than ULONG_MAX, was cho‐
              sen as the default to prevent some cases where  historical
              applications  simply  raised  the  existing  limit without
              first checking its current value.  Such applications would
              cause  the  value  to  overflow  if  the  limit was set at
              ULONG_MAX.

              From Linux 2.4 up to Linux 3.15,  the  default  value  for
              this limit was:

                  SHMMAX / PAGE_SIZE * (SHMMNI / 16)

              If  SHMMAX  and SHMMNI were not modified, then multiplying
              the result of this formula by the  page  size  (to  get  a
              value  in  bytes)  yielded a value of 8 GB as the limit on
              the total memory used by all shared memory segments.

       SHMMAX Maximum size in bytes for a shared memory segment.

              On  Linux,  this  limit  can  be  read  and  modified  via
              /proc/sys/kernel/shmmax.   Since  Linux  3.16, the default
              value for this limit is:

                  ULONG_MAX - 2^24

              The effect of this  value  (which  is  suitable  for  both
              32-bit  and  64-bit systems) is to impose no limitation on
              allocations.  See the description of SHMALL for a  discus‐
              sion  of why this default value (rather than ULONG_MAX) is
              used.

              From Linux 2.2 up to Linux 3.15, the default value of this
              limit was 0x2000000 (32MB).

              Because  it  is  not possible to map just part of a shared
              memory  segment,  the  amount  of  virtual  memory  places
              another limit on the maximum size of a usable segment: for
              example, on i386 the largest segments that can  be  mapped
              have  a  size of around 2.8 GB, and on x86_64 the limit is
              around 127 TB.



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 0/4] ipc/shm.c: increase the limits for SHMMAX, SHMALL

[Davidlohr Bueso]

On Tue, 2014-09-23 at 07:24 +0200, Michael Kerrisk (man-pages) wrote:
> David,
> 
> I applied various pieces from your patch on top of material
> that I already had, so that now we have the text below describing
> these limits.  Comments/suggestions/improvements from all welcome.

Looks good, thanks!