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
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 <[email protected]>
---
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
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 <[email protected]>
Reported-by: Davidlohr Bueso <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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(¤t->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
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
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
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
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 <[email protected]>
Acked-by: Davidlohr Bueso <[email protected]>
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 <[email protected]>
Acked-by: Davidlohr Bueso <[email protected]>
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 <[email protected]>
Acked-by: Davidlohr Bueso <[email protected]>
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 <[email protected]>
> Reported-by: Davidlohr Bueso <[email protected]>
> Cc: [email protected]
Signed-off-by: Davidlohr Bueso <[email protected]>
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
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 <[email protected]>
> Reported-by: Davidlohr Bueso <[email protected]>
> Cc: [email protected]
> ---
> 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 */
>
>
> > 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 <[email protected]>
>
> Acked-by: Davidlohr Bueso <[email protected]>
Acked-by: KOSAKI Motohiro <[email protected]>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> -----Original Message-----
> From: Manfred Spraul [mailto:[email protected]]
> Sent: Monday, April 21, 2014 10:27 AM
> To: Davidlohr Bueso; Michael Kerrisk; Martin Schwidefsky
> Cc: LKML; Andrew Morton; KAMEZAWA Hiroyuki; Motohiro Kosaki JP; [email protected]; [email protected]; [email protected];
> 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 <[email protected]>
> Reported-by: Davidlohr Bueso <[email protected]>
> Cc: [email protected]
Acked-by: KOSAKI Motohiro <[email protected]>
> > 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 <[email protected]>
>
> Acked-by: Davidlohr Bueso <[email protected]>
Acked-by: KOSAKI Motohiro <[email protected]>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> > 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 <[email protected]>
>
> Acked-by: Davidlohr Bueso <[email protected]>
Acked-by: KOSAKI Motohiro <[email protected]>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
- Breakup long function names/args.
- Cleaup variable declaration.
- s/current->mm/mm
Signed-off-by: Davidlohr Bueso <[email protected]>
---
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(¤t->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(¤t->mm->mmap_sem);
+ up_write(&mm->mmap_sem);
if (populate)
mm_populate(addr, populate);
--
1.8.1.4
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 <[email protected]>
> ---
> 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(¤t->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 <[email protected]>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
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 <[email protected]>
> Reported-by: Davidlohr Bueso <[email protected]>
> Cc: [email protected]
> ---
> 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 <[email protected]>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
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 <[email protected]>
> ---
> 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(¤t->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(¤t->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/
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
>>> ---
>>> 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/
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 <[email protected]>
Signed-off-by: Manfred Spraul <[email protected]>
> @@ -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
On Tue, 22 Apr 2014 22:25:45 -0700 Davidlohr Bueso <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
On Wed, 23 Apr 2014 15:27:55 -0700 Andrew Morton <[email protected]> wrote:
>
> On Tue, 22 Apr 2014 22:25:45 -0700 Davidlohr Bueso <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 [email protected]
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 <[email protected]>
>>> ---
>>> 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/
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 <[email protected]>
> >>> ---
> >>> 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
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
> <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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