Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-12 Thread Davidlohr Bueso
On Sat, 2014-04-12 at 10:50 +0200, Manfred Spraul wrote:
> On 04/11/2014 10:27 PM, Davidlohr Bueso wrote:
> > On Fri, 2014-04-11 at 20:28 +0200, Manfred Spraul wrote:
> >> Hi Davidlohr,
> >>
> >> On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
> >>> The default size for shmmax is, and always has been, 32Mb.
> >>> Today, in the XXI century, it seems that this value is rather small,
> >>> making users have to increase it via sysctl, which can cause
> >>> unnecessary work and userspace application workarounds[1].
> >>>
> >>> [snip]
> >>> Running this patch through LTP, everything passes, except the following,
> >>> which, due to the nature of this change, is quite expected:
> >>>
> >>> shmget021  TFAIL  :  call succeeded unexpectedly
> >> Why is this TFAIL expected?
> > So looking at shmget02.c, this is the case that fails:
> >
> > for (i = 0; i < TST_TOTAL; i++) {
> > /*
> >  * Look for a failure ...
> >  */
> >
> > TEST(shmget(*(TC[i].skey), TC[i].size, TC[i].flags));
> >
> > if (TEST_RETURN != -1) {
> > tst_resm(TFAIL, "call succeeded unexpectedly");
> > continue;
> > }
> >
> > Where TC[0] is:
> > struct test_case_t {
> > int *skey;
> > int size;
> > int flags;
> > int error;
> > } TC[] = {
> > /* EINVAL - size is 0 */
> > {
> > , 0, IPC_CREAT | IPC_EXCL | SHM_RW, EINVAL},
> >
> > So it's expected because now 0 is actually valid. And before:
> >
> >   EINVAL A new segment was to be created and size < SHMMIN or size > SHMMAX
> >
> >>> diff --git a/ipc/shm.c b/ipc/shm.c
> >>> index 7645961..ae01ffa 100644
> >>> --- a/ipc/shm.c
> >>> +++ b/ipc/shm.c
> >>> @@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
> >>> ipc_params *params)
> >>>   int id;
> >>>   vm_flags_t acctflag = 0;
> >>>
> >>> - if (size < SHMMIN || size > ns->shm_ctlmax)
> >>> + if (ns->shm_ctlmax &&
> >>> + (size < SHMMIN || size > ns->shm_ctlmax))
> >>>   return -EINVAL;
> >>>
> >>> - if (ns->shm_tot + numpages > ns->shm_ctlall)
> >>> + if (ns->shm_ctlall &&
> >>> + ns->shm_tot + numpages > ns->shm_ctlall)
> >>>   return -ENOSPC;
> >>>
> >>>   shp = ipc_rcu_alloc(sizeof(*shp));
> >> Ok, I understand it:
> >> Your patch disables checking shmmax, shmall *AND* checking for SHMMIN.
> > Right, if shmmax is 0, then there's no point checking for shmmin,
> > otherwise we'd always end up returning EINVAL.
> >
> >> a) Have you double checked that 0-sized shm segments work properly?
> >>Does the swap code handle it properly, ...? EINVAL A new segment was to 
> >> be created and size < SHMMIN or size > SHMMAX
> > Hmm so I've been using this patch just fine on my laptop since I sent
> > it. So far I haven't seen any issues. Are you refering to something in
> > particular? I'd be happy to run any cases you're concerned with.
> I'm thinking about malicious applications.
> Create 0-sized segments and then map them. Does find_vma_intersection 
> handle that case?
> The same for all other functions that are called by the shm code.

Right I agree, which is why I corrected it in v2.

> You can't replace code review by "runs for a month"

Manfred, I was not referring to that at all.

> >> b) It's that yet another risk for user space incompatibility?
> > Sorry, I don't follow here.
> Applications expect that shmget(,0,) fails.

Again, v2.

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-12 Thread Manfred Spraul

On 04/11/2014 10:27 PM, Davidlohr Bueso wrote:

On Fri, 2014-04-11 at 20:28 +0200, Manfred Spraul wrote:

Hi Davidlohr,

On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:

The default size for shmmax is, and always has been, 32Mb.
Today, in the XXI century, it seems that this value is rather small,
making users have to increase it via sysctl, which can cause
unnecessary work and userspace application workarounds[1].

[snip]
Running this patch through LTP, everything passes, except the following,
which, due to the nature of this change, is quite expected:

shmget021  TFAIL  :  call succeeded unexpectedly

Why is this TFAIL expected?

So looking at shmget02.c, this is the case that fails:

for (i = 0; i < TST_TOTAL; i++) {
/*
 * Look for a failure ...
 */

TEST(shmget(*(TC[i].skey), TC[i].size, TC[i].flags));

if (TEST_RETURN != -1) {
tst_resm(TFAIL, "call succeeded unexpectedly");
continue;
}

Where TC[0] is:
struct test_case_t {
int *skey;
int size;
int flags;
int error;
} TC[] = {
/* EINVAL - size is 0 */
{
, 0, IPC_CREAT | IPC_EXCL | SHM_RW, EINVAL},

So it's expected because now 0 is actually valid. And before:

  EINVAL A new segment was to be created and size < SHMMIN or size > SHMMAX


diff --git a/ipc/shm.c b/ipc/shm.c
index 7645961..ae01ffa 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
ipc_params *params)
int id;
vm_flags_t acctflag = 0;
   
-	if (size < SHMMIN || size > ns->shm_ctlmax)

+   if (ns->shm_ctlmax &&
+   (size < SHMMIN || size > ns->shm_ctlmax))
return -EINVAL;
   
-	if (ns->shm_tot + numpages > ns->shm_ctlall)

+   if (ns->shm_ctlall &&
+   ns->shm_tot + numpages > ns->shm_ctlall)
return -ENOSPC;
   
   	shp = ipc_rcu_alloc(sizeof(*shp));

Ok, I understand it:
Your patch disables checking shmmax, shmall *AND* checking for SHMMIN.

Right, if shmmax is 0, then there's no point checking for shmmin,
otherwise we'd always end up returning EINVAL.


a) Have you double checked that 0-sized shm segments work properly?
   Does the swap code handle it properly, ...? EINVAL A new segment was to be created 
and size < SHMMIN or size > SHMMAX

Hmm so I've been using this patch just fine on my laptop since I sent
it. So far I haven't seen any issues. Are you refering to something in
particular? I'd be happy to run any cases you're concerned with.

I'm thinking about malicious applications.
Create 0-sized segments and then map them. Does find_vma_intersection 
handle that case?

The same for all other functions that are called by the shm code.

You can't replace code review by "runs for a month"

b) It's that yet another risk for user space incompatibility?

Sorry, I don't follow here.

Applications expect that shmget(,0,) fails.

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-12 Thread Manfred Spraul

On 04/11/2014 10:27 PM, Davidlohr Bueso wrote:

On Fri, 2014-04-11 at 20:28 +0200, Manfred Spraul wrote:

Hi Davidlohr,

On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:

The default size for shmmax is, and always has been, 32Mb.
Today, in the XXI century, it seems that this value is rather small,
making users have to increase it via sysctl, which can cause
unnecessary work and userspace application workarounds[1].

[snip]
Running this patch through LTP, everything passes, except the following,
which, due to the nature of this change, is quite expected:

shmget021  TFAIL  :  call succeeded unexpectedly

Why is this TFAIL expected?

So looking at shmget02.c, this is the case that fails:

for (i = 0; i  TST_TOTAL; i++) {
/*
 * Look for a failure ...
 */

TEST(shmget(*(TC[i].skey), TC[i].size, TC[i].flags));

if (TEST_RETURN != -1) {
tst_resm(TFAIL, call succeeded unexpectedly);
continue;
}

Where TC[0] is:
struct test_case_t {
int *skey;
int size;
int flags;
int error;
} TC[] = {
/* EINVAL - size is 0 */
{
shmkey2, 0, IPC_CREAT | IPC_EXCL | SHM_RW, EINVAL},

So it's expected because now 0 is actually valid. And before:

  EINVAL A new segment was to be created and size  SHMMIN or size  SHMMAX


diff --git a/ipc/shm.c b/ipc/shm.c
index 7645961..ae01ffa 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
ipc_params *params)
int id;
vm_flags_t acctflag = 0;
   
-	if (size  SHMMIN || size  ns-shm_ctlmax)

+   if (ns-shm_ctlmax 
+   (size  SHMMIN || size  ns-shm_ctlmax))
return -EINVAL;
   
-	if (ns-shm_tot + numpages  ns-shm_ctlall)

+   if (ns-shm_ctlall 
+   ns-shm_tot + numpages  ns-shm_ctlall)
return -ENOSPC;
   
   	shp = ipc_rcu_alloc(sizeof(*shp));

Ok, I understand it:
Your patch disables checking shmmax, shmall *AND* checking for SHMMIN.

Right, if shmmax is 0, then there's no point checking for shmmin,
otherwise we'd always end up returning EINVAL.


a) Have you double checked that 0-sized shm segments work properly?
   Does the swap code handle it properly, ...? EINVAL A new segment was to be created 
and size  SHMMIN or size  SHMMAX

Hmm so I've been using this patch just fine on my laptop since I sent
it. So far I haven't seen any issues. Are you refering to something in
particular? I'd be happy to run any cases you're concerned with.

I'm thinking about malicious applications.
Create 0-sized segments and then map them. Does find_vma_intersection 
handle that case?

The same for all other functions that are called by the shm code.

You can't replace code review by runs for a month

b) It's that yet another risk for user space incompatibility?

Sorry, I don't follow here.

Applications expect that shmget(,0,) fails.

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-12 Thread Davidlohr Bueso
On Sat, 2014-04-12 at 10:50 +0200, Manfred Spraul wrote:
 On 04/11/2014 10:27 PM, Davidlohr Bueso wrote:
  On Fri, 2014-04-11 at 20:28 +0200, Manfred Spraul wrote:
  Hi Davidlohr,
 
  On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
  The default size for shmmax is, and always has been, 32Mb.
  Today, in the XXI century, it seems that this value is rather small,
  making users have to increase it via sysctl, which can cause
  unnecessary work and userspace application workarounds[1].
 
  [snip]
  Running this patch through LTP, everything passes, except the following,
  which, due to the nature of this change, is quite expected:
 
  shmget021  TFAIL  :  call succeeded unexpectedly
  Why is this TFAIL expected?
  So looking at shmget02.c, this is the case that fails:
 
  for (i = 0; i  TST_TOTAL; i++) {
  /*
   * Look for a failure ...
   */
 
  TEST(shmget(*(TC[i].skey), TC[i].size, TC[i].flags));
 
  if (TEST_RETURN != -1) {
  tst_resm(TFAIL, call succeeded unexpectedly);
  continue;
  }
 
  Where TC[0] is:
  struct test_case_t {
  int *skey;
  int size;
  int flags;
  int error;
  } TC[] = {
  /* EINVAL - size is 0 */
  {
  shmkey2, 0, IPC_CREAT | IPC_EXCL | SHM_RW, EINVAL},
 
  So it's expected because now 0 is actually valid. And before:
 
EINVAL A new segment was to be created and size  SHMMIN or size  SHMMAX
 
  diff --git a/ipc/shm.c b/ipc/shm.c
  index 7645961..ae01ffa 100644
  --- a/ipc/shm.c
  +++ b/ipc/shm.c
  @@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
  ipc_params *params)
int id;
vm_flags_t acctflag = 0;
 
  - if (size  SHMMIN || size  ns-shm_ctlmax)
  + if (ns-shm_ctlmax 
  + (size  SHMMIN || size  ns-shm_ctlmax))
return -EINVAL;
 
  - if (ns-shm_tot + numpages  ns-shm_ctlall)
  + if (ns-shm_ctlall 
  + ns-shm_tot + numpages  ns-shm_ctlall)
return -ENOSPC;
 
shp = ipc_rcu_alloc(sizeof(*shp));
  Ok, I understand it:
  Your patch disables checking shmmax, shmall *AND* checking for SHMMIN.
  Right, if shmmax is 0, then there's no point checking for shmmin,
  otherwise we'd always end up returning EINVAL.
 
  a) Have you double checked that 0-sized shm segments work properly?
 Does the swap code handle it properly, ...? EINVAL A new segment was to 
  be created and size  SHMMIN or size  SHMMAX
  Hmm so I've been using this patch just fine on my laptop since I sent
  it. So far I haven't seen any issues. Are you refering to something in
  particular? I'd be happy to run any cases you're concerned with.
 I'm thinking about malicious applications.
 Create 0-sized segments and then map them. Does find_vma_intersection 
 handle that case?
 The same for all other functions that are called by the shm code.

Right I agree, which is why I corrected it in v2.

 You can't replace code review by runs for a month

Manfred, I was not referring to that at all.

  b) It's that yet another risk for user space incompatibility?
  Sorry, I don't follow here.
 Applications expect that shmget(,0,) fails.

Again, v2.

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-11 Thread Davidlohr Bueso
On Fri, 2014-04-11 at 13:27 -0700, Davidlohr Bueso wrote:
> On Fri, 2014-04-11 at 20:28 +0200, Manfred Spraul wrote:
> > Your patch disables checking shmmax, shmall *AND* checking for SHMMIN.
> 
> Right, if shmmax is 0, then there's no point checking for shmmin,
> otherwise we'd always end up returning EINVAL.

Actually that's complete bogus.

Now that I think of it, shmget(key, 0, flg) should still return EINVAL.
That has *nothing* to do with any limits we are changing here and is
simply wrong since the passed size still cannot be less than 1 (shmmin).
I'll update the patch, thanks for pointing this out.

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-11 Thread Davidlohr Bueso
On Fri, 2014-04-11 at 20:28 +0200, Manfred Spraul wrote:
> Hi Davidlohr,
> 
> On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
> > The default size for shmmax is, and always has been, 32Mb.
> > Today, in the XXI century, it seems that this value is rather small,
> > making users have to increase it via sysctl, which can cause
> > unnecessary work and userspace application workarounds[1].
> >
> > [snip]
> > Running this patch through LTP, everything passes, except the following,
> > which, due to the nature of this change, is quite expected:
> >
> > shmget021  TFAIL  :  call succeeded unexpectedly
> Why is this TFAIL expected?

So looking at shmget02.c, this is the case that fails:

for (i = 0; i < TST_TOTAL; i++) {
/*
 * Look for a failure ...
 */

TEST(shmget(*(TC[i].skey), TC[i].size, TC[i].flags));

if (TEST_RETURN != -1) {
tst_resm(TFAIL, "call succeeded unexpectedly");
continue;
}

Where TC[0] is: 
struct test_case_t {
int *skey;
int size;
int flags;
int error;
} TC[] = {
/* EINVAL - size is 0 */
{
, 0, IPC_CREAT | IPC_EXCL | SHM_RW, EINVAL},

So it's expected because now 0 is actually valid. And before:

 EINVAL A new segment was to be created and size < SHMMIN or size > SHMMAX

> >
> > diff --git a/ipc/shm.c b/ipc/shm.c
> > index 7645961..ae01ffa 100644
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
> > ipc_params *params)
> > int id;
> > vm_flags_t acctflag = 0;
> >   
> > -   if (size < SHMMIN || size > ns->shm_ctlmax)
> > +   if (ns->shm_ctlmax &&
> > +   (size < SHMMIN || size > ns->shm_ctlmax))
> > return -EINVAL;
> >   
> > -   if (ns->shm_tot + numpages > ns->shm_ctlall)
> > +   if (ns->shm_ctlall &&
> > +   ns->shm_tot + numpages > ns->shm_ctlall)
> > return -ENOSPC;
> >   
> > shp = ipc_rcu_alloc(sizeof(*shp));
> Ok, I understand it:
> Your patch disables checking shmmax, shmall *AND* checking for SHMMIN.

Right, if shmmax is 0, then there's no point checking for shmmin,
otherwise we'd always end up returning EINVAL.

> 
> a) Have you double checked that 0-sized shm segments work properly?
>   Does the swap code handle it properly, ...? EINVAL A new segment was to be 
> created and size < SHMMIN or size > SHMMAX

Hmm so I've been using this patch just fine on my laptop since I sent
it. So far I haven't seen any issues. Are you refering to something in
particular? I'd be happy to run any cases you're concerned with.

> b) It's that yet another risk for user space incompatibility?

Sorry, I don't follow here.

> c) The patch summary is misleading, the impact on SHMMIN is not mentioned.

Sure, I can explicitly add it to the changelog.

Thanks,
Davidlohr


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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-11 Thread Manfred Spraul

Hi Davidlohr,

On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:

The default size for shmmax is, and always has been, 32Mb.
Today, in the XXI century, it seems that this value is rather small,
making users have to increase it via sysctl, which can cause
unnecessary work and userspace application workarounds[1].

[snip]
Running this patch through LTP, everything passes, except the following,
which, due to the nature of this change, is quite expected:

shmget021  TFAIL  :  call succeeded unexpectedly

Why is this TFAIL expected?


diff --git a/ipc/shm.c b/ipc/shm.c
index 7645961..ae01ffa 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
ipc_params *params)
int id;
vm_flags_t acctflag = 0;
  
-	if (size < SHMMIN || size > ns->shm_ctlmax)

+   if (ns->shm_ctlmax &&
+   (size < SHMMIN || size > ns->shm_ctlmax))
return -EINVAL;
  
-	if (ns->shm_tot + numpages > ns->shm_ctlall)

+   if (ns->shm_ctlall &&
+   ns->shm_tot + numpages > ns->shm_ctlall)
return -ENOSPC;
  
  	shp = ipc_rcu_alloc(sizeof(*shp));

Ok, I understand it:
Your patch disables checking shmmax, shmall *AND* checking for SHMMIN.

a) Have you double checked that 0-sized shm segments work properly?
 Does the swap code handle it properly, ...?

b) It's that yet another risk for user space incompatibility?

c) The patch summary is misleading, the impact on SHMMIN is not mentioned.

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-11 Thread Manfred Spraul

Hi Davidlohr,

On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:

The default size for shmmax is, and always has been, 32Mb.
Today, in the XXI century, it seems that this value is rather small,
making users have to increase it via sysctl, which can cause
unnecessary work and userspace application workarounds[1].

[snip]
Running this patch through LTP, everything passes, except the following,
which, due to the nature of this change, is quite expected:

shmget021  TFAIL  :  call succeeded unexpectedly

Why is this TFAIL expected?


diff --git a/ipc/shm.c b/ipc/shm.c
index 7645961..ae01ffa 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
ipc_params *params)
int id;
vm_flags_t acctflag = 0;
  
-	if (size  SHMMIN || size  ns-shm_ctlmax)

+   if (ns-shm_ctlmax 
+   (size  SHMMIN || size  ns-shm_ctlmax))
return -EINVAL;
  
-	if (ns-shm_tot + numpages  ns-shm_ctlall)

+   if (ns-shm_ctlall 
+   ns-shm_tot + numpages  ns-shm_ctlall)
return -ENOSPC;
  
  	shp = ipc_rcu_alloc(sizeof(*shp));

Ok, I understand it:
Your patch disables checking shmmax, shmall *AND* checking for SHMMIN.

a) Have you double checked that 0-sized shm segments work properly?
 Does the swap code handle it properly, ...?

b) It's that yet another risk for user space incompatibility?

c) The patch summary is misleading, the impact on SHMMIN is not mentioned.

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-11 Thread Davidlohr Bueso
On Fri, 2014-04-11 at 20:28 +0200, Manfred Spraul wrote:
 Hi Davidlohr,
 
 On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
  The default size for shmmax is, and always has been, 32Mb.
  Today, in the XXI century, it seems that this value is rather small,
  making users have to increase it via sysctl, which can cause
  unnecessary work and userspace application workarounds[1].
 
  [snip]
  Running this patch through LTP, everything passes, except the following,
  which, due to the nature of this change, is quite expected:
 
  shmget021  TFAIL  :  call succeeded unexpectedly
 Why is this TFAIL expected?

So looking at shmget02.c, this is the case that fails:

for (i = 0; i  TST_TOTAL; i++) {
/*
 * Look for a failure ...
 */

TEST(shmget(*(TC[i].skey), TC[i].size, TC[i].flags));

if (TEST_RETURN != -1) {
tst_resm(TFAIL, call succeeded unexpectedly);
continue;
}

Where TC[0] is: 
struct test_case_t {
int *skey;
int size;
int flags;
int error;
} TC[] = {
/* EINVAL - size is 0 */
{
shmkey2, 0, IPC_CREAT | IPC_EXCL | SHM_RW, EINVAL},

So it's expected because now 0 is actually valid. And before:

 EINVAL A new segment was to be created and size  SHMMIN or size  SHMMAX

 
  diff --git a/ipc/shm.c b/ipc/shm.c
  index 7645961..ae01ffa 100644
  --- a/ipc/shm.c
  +++ b/ipc/shm.c
  @@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
  ipc_params *params)
  int id;
  vm_flags_t acctflag = 0;

  -   if (size  SHMMIN || size  ns-shm_ctlmax)
  +   if (ns-shm_ctlmax 
  +   (size  SHMMIN || size  ns-shm_ctlmax))
  return -EINVAL;

  -   if (ns-shm_tot + numpages  ns-shm_ctlall)
  +   if (ns-shm_ctlall 
  +   ns-shm_tot + numpages  ns-shm_ctlall)
  return -ENOSPC;

  shp = ipc_rcu_alloc(sizeof(*shp));
 Ok, I understand it:
 Your patch disables checking shmmax, shmall *AND* checking for SHMMIN.

Right, if shmmax is 0, then there's no point checking for shmmin,
otherwise we'd always end up returning EINVAL.

 
 a) Have you double checked that 0-sized shm segments work properly?
   Does the swap code handle it properly, ...? EINVAL A new segment was to be 
 created and size  SHMMIN or size  SHMMAX

Hmm so I've been using this patch just fine on my laptop since I sent
it. So far I haven't seen any issues. Are you refering to something in
particular? I'd be happy to run any cases you're concerned with.

 b) It's that yet another risk for user space incompatibility?

Sorry, I don't follow here.

 c) The patch summary is misleading, the impact on SHMMIN is not mentioned.

Sure, I can explicitly add it to the changelog.

Thanks,
Davidlohr


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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-11 Thread Davidlohr Bueso
On Fri, 2014-04-11 at 13:27 -0700, Davidlohr Bueso wrote:
 On Fri, 2014-04-11 at 20:28 +0200, Manfred Spraul wrote:
  Your patch disables checking shmmax, shmall *AND* checking for SHMMIN.
 
 Right, if shmmax is 0, then there's no point checking for shmmin,
 otherwise we'd always end up returning EINVAL.

Actually that's complete bogus.

Now that I think of it, shmget(key, 0, flg) should still return EINVAL.
That has *nothing* to do with any limits we are changing here and is
simply wrong since the passed size still cannot be less than 1 (shmmin).
I'll update the patch, thanks for pointing this out.

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-06 Thread Davidlohr Bueso
On Sun, 2014-04-06 at 08:42 +0200, Manfred Spraul wrote:
> Hi,
> 
> On 04/05/2014 08:24 PM, KOSAKI Motohiro wrote:
> > On Fri, Apr 4, 2014 at 1:00 AM, Davidlohr Bueso  wrote:
> >> I don't think it makes much sense to set unlimited for both 0 and
> >> ULONG_MAX, that would probably just create even more confusion.
> I agree.
> Unlimited was INT_MAX since 0.99.10 and ULONG_MAX since 2.3.39 (with 
> proper backward compatibility for user space).
> 
> Adding a second value for unlimited just creates confusion.
> >> But then again, we shouldn't even care about breaking things with shmmax
> >> or shmall with 0 value, it just makes no sense from a user PoV. shmmax
> >> cannot be 0 unless there's an overflow, which voids any valid cases, and
> >> thus shmall cannot be 0 either as it would go against any values set for
> >> shmmax. I think it's safe to ignore this.
> > Agreed.
> > IMHO, until you find out any incompatibility issue of this, we don't
> > need the switch
> > because we can't make good workaround for that. I'd suggest to merge your 
> > patch
> > and see what happen.
> I disagree:
> - "shmctl(,IPC_INFO,); if (my_memory_size > buf.shmmax) 
> perror("change shmmax");" worked correctly since 0.99.10. I don't think 
> that merging the patch and seeing what happens is the right approach.

I agree, we *must* get this right the first time. So no rushing into
things that might later come and bite us in the future.

That said, if users are doing that kind of check, then they must also
check against shmmin, which has _always_ been 1. So shmmax == 0 is a no
no. Otherwise it's not the kernel's fault that they're misusing the API,
which IMO is pretty straightforward for such things. And if shmmax
cannot be 0, shmall cannot be 0.

> - setting shmmax by default to ULONG_MAX is the perfect workaround.
> 
> What reasons are there against the one-line patch?

There's really nothing wrong with it, it's just that 0 is a much nicer
value to have for 'unlimited'. And if we can get away with it, then
lets, otherwise yes, we should go with this path.



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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-06 Thread Manfred Spraul

Hi,

On 04/05/2014 08:24 PM, KOSAKI Motohiro wrote:

On Fri, Apr 4, 2014 at 1:00 AM, Davidlohr Bueso  wrote:

I don't think it makes much sense to set unlimited for both 0 and
ULONG_MAX, that would probably just create even more confusion.

I agree.
Unlimited was INT_MAX since 0.99.10 and ULONG_MAX since 2.3.39 (with 
proper backward compatibility for user space).


Adding a second value for unlimited just creates confusion.

But then again, we shouldn't even care about breaking things with shmmax
or shmall with 0 value, it just makes no sense from a user PoV. shmmax
cannot be 0 unless there's an overflow, which voids any valid cases, and
thus shmall cannot be 0 either as it would go against any values set for
shmmax. I think it's safe to ignore this.

Agreed.
IMHO, until you find out any incompatibility issue of this, we don't
need the switch
because we can't make good workaround for that. I'd suggest to merge your patch
and see what happen.

I disagree:
- "shmctl(,IPC_INFO,); if (my_memory_size > buf.shmmax) 
perror("change shmmax");" worked correctly since 0.99.10. I don't think 
that merging the patch and seeing what happens is the right approach.

- setting shmmax by default to ULONG_MAX is the perfect workaround.

What reasons are there against the one-line patch?
>
> -#define SHMMAX 0x200/* max shared seg size 
(bytes) */
> +#define SHMMAX ULONG_MAX/* max shared seg size 
(bytes) */

>

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-06 Thread Manfred Spraul

Hi,

On 04/05/2014 08:24 PM, KOSAKI Motohiro wrote:

On Fri, Apr 4, 2014 at 1:00 AM, Davidlohr Bueso davidl...@hp.com wrote:

I don't think it makes much sense to set unlimited for both 0 and
ULONG_MAX, that would probably just create even more confusion.

I agree.
Unlimited was INT_MAX since 0.99.10 and ULONG_MAX since 2.3.39 (with 
proper backward compatibility for user space).


Adding a second value for unlimited just creates confusion.

But then again, we shouldn't even care about breaking things with shmmax
or shmall with 0 value, it just makes no sense from a user PoV. shmmax
cannot be 0 unless there's an overflow, which voids any valid cases, and
thus shmall cannot be 0 either as it would go against any values set for
shmmax. I think it's safe to ignore this.

Agreed.
IMHO, until you find out any incompatibility issue of this, we don't
need the switch
because we can't make good workaround for that. I'd suggest to merge your patch
and see what happen.

I disagree:
- shmctl(,IPC_INFO,buf); if (my_memory_size  buf.shmmax) 
perror(change shmmax); worked correctly since 0.99.10. I don't think 
that merging the patch and seeing what happens is the right approach.

- setting shmmax by default to ULONG_MAX is the perfect workaround.

What reasons are there against the one-line patch?

 -#define SHMMAX 0x200/* max shared seg size 
(bytes) */
 +#define SHMMAX ULONG_MAX/* max shared seg size 
(bytes) */



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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-06 Thread Davidlohr Bueso
On Sun, 2014-04-06 at 08:42 +0200, Manfred Spraul wrote:
 Hi,
 
 On 04/05/2014 08:24 PM, KOSAKI Motohiro wrote:
  On Fri, Apr 4, 2014 at 1:00 AM, Davidlohr Bueso davidl...@hp.com wrote:
  I don't think it makes much sense to set unlimited for both 0 and
  ULONG_MAX, that would probably just create even more confusion.
 I agree.
 Unlimited was INT_MAX since 0.99.10 and ULONG_MAX since 2.3.39 (with 
 proper backward compatibility for user space).
 
 Adding a second value for unlimited just creates confusion.
  But then again, we shouldn't even care about breaking things with shmmax
  or shmall with 0 value, it just makes no sense from a user PoV. shmmax
  cannot be 0 unless there's an overflow, which voids any valid cases, and
  thus shmall cannot be 0 either as it would go against any values set for
  shmmax. I think it's safe to ignore this.
  Agreed.
  IMHO, until you find out any incompatibility issue of this, we don't
  need the switch
  because we can't make good workaround for that. I'd suggest to merge your 
  patch
  and see what happen.
 I disagree:
 - shmctl(,IPC_INFO,buf); if (my_memory_size  buf.shmmax) 
 perror(change shmmax); worked correctly since 0.99.10. I don't think 
 that merging the patch and seeing what happens is the right approach.

I agree, we *must* get this right the first time. So no rushing into
things that might later come and bite us in the future.

That said, if users are doing that kind of check, then they must also
check against shmmin, which has _always_ been 1. So shmmax == 0 is a no
no. Otherwise it's not the kernel's fault that they're misusing the API,
which IMO is pretty straightforward for such things. And if shmmax
cannot be 0, shmall cannot be 0.

 - setting shmmax by default to ULONG_MAX is the perfect workaround.
 
 What reasons are there against the one-line patch?

There's really nothing wrong with it, it's just that 0 is a much nicer
value to have for 'unlimited'. And if we can get away with it, then
lets, otherwise yes, we should go with this path.



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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-05 Thread KOSAKI Motohiro
On Fri, Apr 4, 2014 at 1:00 AM, Davidlohr Bueso  wrote:
> On Thu, 2014-04-03 at 19:39 -0400, KOSAKI Motohiro wrote:
>> On Thu, Apr 3, 2014 at 3:50 PM, Davidlohr Bueso  wrote:
>> > On Thu, 2014-04-03 at 21:02 +0200, Manfred Spraul wrote:
>> >> Hi Davidlohr,
>> >>
>> >> On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
>> >> > The default size for shmmax is, and always has been, 32Mb.
>> >> > Today, in the XXI century, it seems that this value is rather small,
>> >> > making users have to increase it via sysctl, which can cause
>> >> > unnecessary work and userspace application workarounds[1].
>> >> >
>> >> > Instead of choosing yet another arbitrary value, larger than 32Mb,
>> >> > this patch disables the use of both shmmax and shmall by default,
>> >> > allowing users to create segments of unlimited sizes. Users and
>> >> > applications that already explicitly set these values through sysctl
>> >> > are left untouched, and thus does not change any of the behavior.
>> >> >
>> >> > So a value of 0 bytes or pages, for shmmax and shmall, respectively,
>> >> > implies unlimited memory, as opposed to disabling sysv shared memory.
>> >> > This is safe as 0 cannot possibly be used previously as SHMMIN is
>> >> > hardcoded to 1 and cannot be modified.
>> >
>> >> Are we sure that no user space apps uses shmctl(IPC_INFO) and prints a
>> >> pretty error message if shmall is too small?
>> >> We would break these apps.
>> >
>> > Good point. 0 bytes/pages would definitely trigger an unexpected error
>> > message if users did this. But on the other hand I'm not sure this
>> > actually is a _real_ scenario, since upon overflow the value can still
>> > end up being 0, which is totally bogus and would cause the same
>> > breakage.
>> >
>> > So I see two possible workarounds:
>> > (i) Use ULONG_MAX for the shmmax default instead. This would make shmall
>> > default to 1152921504606846720 and 268435456, for 64 and 32bit systems,
>> > respectively.
>> >
>> > (ii) Keep the 0 bytes, but add a new a "transition" tunable that, if set
>> > (default off), would allow 0 bytes to be unlimited. With time, users
>> > could hopefully update their applications and we could eventually get
>> > rid of it. This _seems_ to be the less aggressive way to go.
>>
>> Do you mean
>>
>> set 0: IPC_INFO return shmmax = 0.
>> set 1: IPC_INFO return shmmax = ULONG_MAX.
>>
>> ?
>>
>> That makes sense.
>
> Well I was mostly referring to:
>
> set 0: leave things as there are now.
> set 1: this patch.

I don't recommend this approach because many user never switch 1 and
finally getting API fragmentation.


> I don't think it makes much sense to set unlimited for both 0 and
> ULONG_MAX, that would probably just create even more confusion.
>
> But then again, we shouldn't even care about breaking things with shmmax
> or shmall with 0 value, it just makes no sense from a user PoV. shmmax
> cannot be 0 unless there's an overflow, which voids any valid cases, and
> thus shmall cannot be 0 either as it would go against any values set for
> shmmax. I think it's safe to ignore this.

Agreed.
IMHO, until you find out any incompatibility issue of this, we don't
need the switch
because we can't make good workaround for that. I'd suggest to merge your patch
and see what happen.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-05 Thread KOSAKI Motohiro
On Fri, Apr 4, 2014 at 1:00 AM, Davidlohr Bueso davidl...@hp.com wrote:
 On Thu, 2014-04-03 at 19:39 -0400, KOSAKI Motohiro wrote:
 On Thu, Apr 3, 2014 at 3:50 PM, Davidlohr Bueso davidl...@hp.com wrote:
  On Thu, 2014-04-03 at 21:02 +0200, Manfred Spraul wrote:
  Hi Davidlohr,
 
  On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
   The default size for shmmax is, and always has been, 32Mb.
   Today, in the XXI century, it seems that this value is rather small,
   making users have to increase it via sysctl, which can cause
   unnecessary work and userspace application workarounds[1].
  
   Instead of choosing yet another arbitrary value, larger than 32Mb,
   this patch disables the use of both shmmax and shmall by default,
   allowing users to create segments of unlimited sizes. Users and
   applications that already explicitly set these values through sysctl
   are left untouched, and thus does not change any of the behavior.
  
   So a value of 0 bytes or pages, for shmmax and shmall, respectively,
   implies unlimited memory, as opposed to disabling sysv shared memory.
   This is safe as 0 cannot possibly be used previously as SHMMIN is
   hardcoded to 1 and cannot be modified.
 
  Are we sure that no user space apps uses shmctl(IPC_INFO) and prints a
  pretty error message if shmall is too small?
  We would break these apps.
 
  Good point. 0 bytes/pages would definitely trigger an unexpected error
  message if users did this. But on the other hand I'm not sure this
  actually is a _real_ scenario, since upon overflow the value can still
  end up being 0, which is totally bogus and would cause the same
  breakage.
 
  So I see two possible workarounds:
  (i) Use ULONG_MAX for the shmmax default instead. This would make shmall
  default to 1152921504606846720 and 268435456, for 64 and 32bit systems,
  respectively.
 
  (ii) Keep the 0 bytes, but add a new a transition tunable that, if set
  (default off), would allow 0 bytes to be unlimited. With time, users
  could hopefully update their applications and we could eventually get
  rid of it. This _seems_ to be the less aggressive way to go.

 Do you mean

 set 0: IPC_INFO return shmmax = 0.
 set 1: IPC_INFO return shmmax = ULONG_MAX.

 ?

 That makes sense.

 Well I was mostly referring to:

 set 0: leave things as there are now.
 set 1: this patch.

I don't recommend this approach because many user never switch 1 and
finally getting API fragmentation.


 I don't think it makes much sense to set unlimited for both 0 and
 ULONG_MAX, that would probably just create even more confusion.

 But then again, we shouldn't even care about breaking things with shmmax
 or shmall with 0 value, it just makes no sense from a user PoV. shmmax
 cannot be 0 unless there's an overflow, which voids any valid cases, and
 thus shmall cannot be 0 either as it would go against any values set for
 shmmax. I think it's safe to ignore this.

Agreed.
IMHO, until you find out any incompatibility issue of this, we don't
need the switch
because we can't make good workaround for that. I'd suggest to merge your patch
and see what happen.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread Davidlohr Bueso
On Thu, 2014-04-03 at 19:39 -0400, KOSAKI Motohiro wrote:
> On Thu, Apr 3, 2014 at 3:50 PM, Davidlohr Bueso  wrote:
> > On Thu, 2014-04-03 at 21:02 +0200, Manfred Spraul wrote:
> >> Hi Davidlohr,
> >>
> >> On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
> >> > The default size for shmmax is, and always has been, 32Mb.
> >> > Today, in the XXI century, it seems that this value is rather small,
> >> > making users have to increase it via sysctl, which can cause
> >> > unnecessary work and userspace application workarounds[1].
> >> >
> >> > Instead of choosing yet another arbitrary value, larger than 32Mb,
> >> > this patch disables the use of both shmmax and shmall by default,
> >> > allowing users to create segments of unlimited sizes. Users and
> >> > applications that already explicitly set these values through sysctl
> >> > are left untouched, and thus does not change any of the behavior.
> >> >
> >> > So a value of 0 bytes or pages, for shmmax and shmall, respectively,
> >> > implies unlimited memory, as opposed to disabling sysv shared memory.
> >> > This is safe as 0 cannot possibly be used previously as SHMMIN is
> >> > hardcoded to 1 and cannot be modified.
> >
> >> Are we sure that no user space apps uses shmctl(IPC_INFO) and prints a
> >> pretty error message if shmall is too small?
> >> We would break these apps.
> >
> > Good point. 0 bytes/pages would definitely trigger an unexpected error
> > message if users did this. But on the other hand I'm not sure this
> > actually is a _real_ scenario, since upon overflow the value can still
> > end up being 0, which is totally bogus and would cause the same
> > breakage.
> >
> > So I see two possible workarounds:
> > (i) Use ULONG_MAX for the shmmax default instead. This would make shmall
> > default to 1152921504606846720 and 268435456, for 64 and 32bit systems,
> > respectively.
> >
> > (ii) Keep the 0 bytes, but add a new a "transition" tunable that, if set
> > (default off), would allow 0 bytes to be unlimited. With time, users
> > could hopefully update their applications and we could eventually get
> > rid of it. This _seems_ to be the less aggressive way to go.
> 
> Do you mean
> 
> set 0: IPC_INFO return shmmax = 0.
> set 1: IPC_INFO return shmmax = ULONG_MAX.
> 
> ?
> 
> That makes sense.

Well I was mostly referring to:

set 0: leave things as there are now.
set 1: this patch.

I don't think it makes much sense to set unlimited for both 0 and
ULONG_MAX, that would probably just create even more confusion. 

But then again, we shouldn't even care about breaking things with shmmax
or shmall with 0 value, it just makes no sense from a user PoV. shmmax
cannot be 0 unless there's an overflow, which voids any valid cases, and
thus shmall cannot be 0 either as it would go against any values set for
shmmax. I think it's safe to ignore this.


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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread KOSAKI Motohiro
> This change allows Linux to treat shm just as regular anonymous memory.
> One important difference between them, though, is handling out-of-memory
> conditions: as opposed to regular anon memory, the OOM killer will not
> kill processes that are hogging memory through shm, allowing users to
> potentially abuse this. To overcome this situation, the shm_rmid_forced
> option must be enabled.

Off topic: systemd implemented similar feature RemoveIPC and it is
enabled by default.
http://lists.freedesktop.org/archives/systemd-devel/2014-March/018232.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread KOSAKI Motohiro
On Thu, Apr 3, 2014 at 3:50 PM, Davidlohr Bueso  wrote:
> On Thu, 2014-04-03 at 21:02 +0200, Manfred Spraul wrote:
>> Hi Davidlohr,
>>
>> On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
>> > The default size for shmmax is, and always has been, 32Mb.
>> > Today, in the XXI century, it seems that this value is rather small,
>> > making users have to increase it via sysctl, which can cause
>> > unnecessary work and userspace application workarounds[1].
>> >
>> > Instead of choosing yet another arbitrary value, larger than 32Mb,
>> > this patch disables the use of both shmmax and shmall by default,
>> > allowing users to create segments of unlimited sizes. Users and
>> > applications that already explicitly set these values through sysctl
>> > are left untouched, and thus does not change any of the behavior.
>> >
>> > So a value of 0 bytes or pages, for shmmax and shmall, respectively,
>> > implies unlimited memory, as opposed to disabling sysv shared memory.
>> > This is safe as 0 cannot possibly be used previously as SHMMIN is
>> > hardcoded to 1 and cannot be modified.
>
>> Are we sure that no user space apps uses shmctl(IPC_INFO) and prints a
>> pretty error message if shmall is too small?
>> We would break these apps.
>
> Good point. 0 bytes/pages would definitely trigger an unexpected error
> message if users did this. But on the other hand I'm not sure this
> actually is a _real_ scenario, since upon overflow the value can still
> end up being 0, which is totally bogus and would cause the same
> breakage.
>
> So I see two possible workarounds:
> (i) Use ULONG_MAX for the shmmax default instead. This would make shmall
> default to 1152921504606846720 and 268435456, for 64 and 32bit systems,
> respectively.
>
> (ii) Keep the 0 bytes, but add a new a "transition" tunable that, if set
> (default off), would allow 0 bytes to be unlimited. With time, users
> could hopefully update their applications and we could eventually get
> rid of it. This _seems_ to be the less aggressive way to go.

Do you mean

set 0: IPC_INFO return shmmax = 0.
set 1: IPC_INFO return shmmax = ULONG_MAX.

?

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread KOSAKI Motohiro
On Wed, Apr 2, 2014 at 8:20 PM, Davidlohr Bueso  wrote:
> The default size for shmmax is, and always has been, 32Mb.
> Today, in the XXI century, it seems that this value is rather small,
> making users have to increase it via sysctl, which can cause
> unnecessary work and userspace application workarounds[1].
>
> Instead of choosing yet another arbitrary value, larger than 32Mb,
> this patch disables the use of both shmmax and shmall by default,
> allowing users to create segments of unlimited sizes. Users and
> applications that already explicitly set these values through sysctl
> are left untouched, and thus does not change any of the behavior.
>
> So a value of 0 bytes or pages, for shmmax and shmall, respectively,
> implies unlimited memory, as opposed to disabling sysv shared memory.
> This is safe as 0 cannot possibly be used previously as SHMMIN is
> hardcoded to 1 and cannot be modified.
>
> This change allows Linux to treat shm just as regular anonymous memory.
> One important difference between them, though, is handling out-of-memory
> conditions: as opposed to regular anon memory, the OOM killer will not
> kill processes that are hogging memory through shm, allowing users to
> potentially abuse this. To overcome this situation, the shm_rmid_forced
> option must be enabled.

I'm very slightly against this sentence.

OOM killer WILL kill the process because shm touching increase RSS anyway.
But the killing doesn't make memory freeing because it's shmem.

>
> Running this patch through LTP, everything passes, except the following,
> which, due to the nature of this change, is quite expected:
>
> shmget021  TFAIL  :  call succeeded unexpectedly
>
> [1]: http://rhaas.blogspot.com/2012/06/absurd-shared-memory-limits.html
>
> Signed-off-by: Davidlohr Bueso 
> ---
>  include/linux/shm.h  | 2 +-
>  include/uapi/linux/shm.h | 8 
>  ipc/shm.c| 6 --
>  3 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/shm.h b/include/linux/shm.h
> index 1e2cd2e..0ca06a3 100644
> --- a/include/linux/shm.h
> +++ b/include/linux/shm.h
> @@ -4,7 +4,7 @@
>  #include 
>  #include 
>
> -#define SHMALL (SHMMAX/PAGE_SIZE*(SHMMNI/16)) /* max shm system wide (pages) 
> */
> +#define SHMALL 0 /* max shm system wide (pages) */
>  #include 
>  struct shmid_kernel /* private to the kernel */
>  {
> diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
> index 78b6941..5f0ef28 100644
> --- a/include/uapi/linux/shm.h
> +++ b/include/uapi/linux/shm.h
> @@ -9,14 +9,14 @@
>
>  /*
>   * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
> - * be increased by sysctl
> + * be increased by sysctl. By default, disable SHMMAX and SHMALL with
> + * 0 bytes, thus allowing processes to have unlimited shared memory.
>   */
> -
> -#define SHMMAX 0x200/* max shared seg size (bytes) */
> +#define SHMMAX 0/* 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))
> +#define SHMALL 0
>  #endif
>  #define SHMSEG SHMMNI   /* max shared segs per process */
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 7645961..ae01ffa 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
> ipc_params *params)
> int id;
> vm_flags_t acctflag = 0;
>
> -   if (size < SHMMIN || size > ns->shm_ctlmax)
> +   if (ns->shm_ctlmax &&
> +   (size < SHMMIN || size > ns->shm_ctlmax))
> return -EINVAL;
>
> -   if (ns->shm_tot + numpages > ns->shm_ctlall)
> +   if (ns->shm_ctlall &&
> +   ns->shm_tot + numpages > ns->shm_ctlall)
> return -ENOSPC;
>
> shp = ipc_rcu_alloc(sizeof(*shp));

Looks good.

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread Davidlohr Bueso
On Thu, 2014-04-03 at 21:02 +0200, Manfred Spraul wrote:
> Hi Davidlohr,
> 
> On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
> > The default size for shmmax is, and always has been, 32Mb.
> > Today, in the XXI century, it seems that this value is rather small,
> > making users have to increase it via sysctl, which can cause
> > unnecessary work and userspace application workarounds[1].
> >
> > Instead of choosing yet another arbitrary value, larger than 32Mb,
> > this patch disables the use of both shmmax and shmall by default,
> > allowing users to create segments of unlimited sizes. Users and
> > applications that already explicitly set these values through sysctl
> > are left untouched, and thus does not change any of the behavior.
> >
> > So a value of 0 bytes or pages, for shmmax and shmall, respectively,
> > implies unlimited memory, as opposed to disabling sysv shared memory.
> > This is safe as 0 cannot possibly be used previously as SHMMIN is
> > hardcoded to 1 and cannot be modified.

> Are we sure that no user space apps uses shmctl(IPC_INFO) and prints a 
> pretty error message if shmall is too small?
> We would break these apps.

Good point. 0 bytes/pages would definitely trigger an unexpected error
message if users did this. But on the other hand I'm not sure this
actually is a _real_ scenario, since upon overflow the value can still
end up being 0, which is totally bogus and would cause the same
breakage.

So I see two possible workarounds:
(i) Use ULONG_MAX for the shmmax default instead. This would make shmall
default to 1152921504606846720 and 268435456, for 64 and 32bit systems,
respectively.

(ii) Keep the 0 bytes, but add a new a "transition" tunable that, if set
(default off), would allow 0 bytes to be unlimited. With time, users
could hopefully update their applications and we could eventually get
rid of it. This _seems_ to be the less aggressive way to go.

Thoughts?

Thanks,
Davidlohr

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread Manfred Spraul

Hi Davidlohr,

On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:

The default size for shmmax is, and always has been, 32Mb.
Today, in the XXI century, it seems that this value is rather small,
making users have to increase it via sysctl, which can cause
unnecessary work and userspace application workarounds[1].

Instead of choosing yet another arbitrary value, larger than 32Mb,
this patch disables the use of both shmmax and shmall by default,
allowing users to create segments of unlimited sizes. Users and
applications that already explicitly set these values through sysctl
are left untouched, and thus does not change any of the behavior.

So a value of 0 bytes or pages, for shmmax and shmall, respectively,
implies unlimited memory, as opposed to disabling sysv shared memory.
This is safe as 0 cannot possibly be used previously as SHMMIN is
hardcoded to 1 and cannot be modified.
Are we sure that no user space apps uses shmctl(IPC_INFO) and prints a 
pretty error message if shmall is too small?

We would break these apps.

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread Kamezawa Hiroyuki

(2014/04/03 9:20), Davidlohr Bueso wrote:

The default size for shmmax is, and always has been, 32Mb.
Today, in the XXI century, it seems that this value is rather small,
making users have to increase it via sysctl, which can cause
unnecessary work and userspace application workarounds[1].

Instead of choosing yet another arbitrary value, larger than 32Mb,
this patch disables the use of both shmmax and shmall by default,
allowing users to create segments of unlimited sizes. Users and
applications that already explicitly set these values through sysctl
are left untouched, and thus does not change any of the behavior.

So a value of 0 bytes or pages, for shmmax and shmall, respectively,
implies unlimited memory, as opposed to disabling sysv shared memory.
This is safe as 0 cannot possibly be used previously as SHMMIN is
hardcoded to 1 and cannot be modified.

This change allows Linux to treat shm just as regular anonymous memory.
One important difference between them, though, is handling out-of-memory
conditions: as opposed to regular anon memory, the OOM killer will not
kill processes that are hogging memory through shm, allowing users to
potentially abuse this. To overcome this situation, the shm_rmid_forced
option must be enabled.

Running this patch through LTP, everything passes, except the following,
which, due to the nature of this change, is quite expected:

shmget021  TFAIL  :  call succeeded unexpectedly

[1]: http://rhaas.blogspot.com/2012/06/absurd-shared-memory-limits.html

Signed-off-by: Davidlohr Bueso 


looks good to me
Acked-by: KAMEZAWA Hiroyuki 

When this goes mainline, updating a man page of ipcs may be required.

Thanks,
-Kame


---
  include/linux/shm.h  | 2 +-
  include/uapi/linux/shm.h | 8 
  ipc/shm.c| 6 --
  3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/linux/shm.h b/include/linux/shm.h
index 1e2cd2e..0ca06a3 100644
--- a/include/linux/shm.h
+++ b/include/linux/shm.h
@@ -4,7 +4,7 @@
  #include 
  #include 

-#define SHMALL (SHMMAX/PAGE_SIZE*(SHMMNI/16)) /* max shm system wide (pages) */
+#define SHMALL 0 /* max shm system wide (pages) */
  #include 
  struct shmid_kernel /* private to the kernel */
  { 
diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
index 78b6941..5f0ef28 100644
--- a/include/uapi/linux/shm.h
+++ b/include/uapi/linux/shm.h
@@ -9,14 +9,14 @@

  /*
   * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
- * be increased by sysctl
+ * be increased by sysctl. By default, disable SHMMAX and SHMALL with
+ * 0 bytes, thus allowing processes to have unlimited shared memory.
   */
-
-#define SHMMAX 0x200/* max shared seg size (bytes) */
+#define SHMMAX 0/* 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))
+#define SHMALL 0
  #endif
  #define SHMSEG SHMMNI  /* max shared segs per process */

diff --git a/ipc/shm.c b/ipc/shm.c
index 7645961..ae01ffa 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
ipc_params *params)
int id;
vm_flags_t acctflag = 0;

-   if (size < SHMMIN || size > ns->shm_ctlmax)
+   if (ns->shm_ctlmax &&
+   (size < SHMMIN || size > ns->shm_ctlmax))
return -EINVAL;

-   if (ns->shm_tot + numpages > ns->shm_ctlall)
+   if (ns->shm_ctlall &&
+   ns->shm_tot + numpages > ns->shm_ctlall)
return -ENOSPC;

shp = ipc_rcu_alloc(sizeof(*shp));




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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread Kamezawa Hiroyuki

(2014/04/03 9:20), Davidlohr Bueso wrote:

The default size for shmmax is, and always has been, 32Mb.
Today, in the XXI century, it seems that this value is rather small,
making users have to increase it via sysctl, which can cause
unnecessary work and userspace application workarounds[1].

Instead of choosing yet another arbitrary value, larger than 32Mb,
this patch disables the use of both shmmax and shmall by default,
allowing users to create segments of unlimited sizes. Users and
applications that already explicitly set these values through sysctl
are left untouched, and thus does not change any of the behavior.

So a value of 0 bytes or pages, for shmmax and shmall, respectively,
implies unlimited memory, as opposed to disabling sysv shared memory.
This is safe as 0 cannot possibly be used previously as SHMMIN is
hardcoded to 1 and cannot be modified.

This change allows Linux to treat shm just as regular anonymous memory.
One important difference between them, though, is handling out-of-memory
conditions: as opposed to regular anon memory, the OOM killer will not
kill processes that are hogging memory through shm, allowing users to
potentially abuse this. To overcome this situation, the shm_rmid_forced
option must be enabled.

Running this patch through LTP, everything passes, except the following,
which, due to the nature of this change, is quite expected:

shmget021  TFAIL  :  call succeeded unexpectedly

[1]: http://rhaas.blogspot.com/2012/06/absurd-shared-memory-limits.html

Signed-off-by: Davidlohr Bueso davidl...@hp.com


looks good to me
Acked-by: KAMEZAWA Hiroyuki kamezawa.hir...@jp.fujitsu.com

When this goes mainline, updating a man page of ipcs may be required.

Thanks,
-Kame


---
  include/linux/shm.h  | 2 +-
  include/uapi/linux/shm.h | 8 
  ipc/shm.c| 6 --
  3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/linux/shm.h b/include/linux/shm.h
index 1e2cd2e..0ca06a3 100644
--- a/include/linux/shm.h
+++ b/include/linux/shm.h
@@ -4,7 +4,7 @@
  #include asm/page.h
  #include uapi/linux/shm.h

-#define SHMALL (SHMMAX/PAGE_SIZE*(SHMMNI/16)) /* max shm system wide (pages) */
+#define SHMALL 0 /* max shm system wide (pages) */
  #include asm/shmparam.h
  struct shmid_kernel /* private to the kernel */
  { 
diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
index 78b6941..5f0ef28 100644
--- a/include/uapi/linux/shm.h
+++ b/include/uapi/linux/shm.h
@@ -9,14 +9,14 @@

  /*
   * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
- * be increased by sysctl
+ * be increased by sysctl. By default, disable SHMMAX and SHMALL with
+ * 0 bytes, thus allowing processes to have unlimited shared memory.
   */
-
-#define SHMMAX 0x200/* max shared seg size (bytes) */
+#define SHMMAX 0/* 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))
+#define SHMALL 0
  #endif
  #define SHMSEG SHMMNI  /* max shared segs per process */

diff --git a/ipc/shm.c b/ipc/shm.c
index 7645961..ae01ffa 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
ipc_params *params)
int id;
vm_flags_t acctflag = 0;

-   if (size  SHMMIN || size  ns-shm_ctlmax)
+   if (ns-shm_ctlmax 
+   (size  SHMMIN || size  ns-shm_ctlmax))
return -EINVAL;

-   if (ns-shm_tot + numpages  ns-shm_ctlall)
+   if (ns-shm_ctlall 
+   ns-shm_tot + numpages  ns-shm_ctlall)
return -ENOSPC;

shp = ipc_rcu_alloc(sizeof(*shp));




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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread Manfred Spraul

Hi Davidlohr,

On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:

The default size for shmmax is, and always has been, 32Mb.
Today, in the XXI century, it seems that this value is rather small,
making users have to increase it via sysctl, which can cause
unnecessary work and userspace application workarounds[1].

Instead of choosing yet another arbitrary value, larger than 32Mb,
this patch disables the use of both shmmax and shmall by default,
allowing users to create segments of unlimited sizes. Users and
applications that already explicitly set these values through sysctl
are left untouched, and thus does not change any of the behavior.

So a value of 0 bytes or pages, for shmmax and shmall, respectively,
implies unlimited memory, as opposed to disabling sysv shared memory.
This is safe as 0 cannot possibly be used previously as SHMMIN is
hardcoded to 1 and cannot be modified.
Are we sure that no user space apps uses shmctl(IPC_INFO) and prints a 
pretty error message if shmall is too small?

We would break these apps.

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread Davidlohr Bueso
On Thu, 2014-04-03 at 21:02 +0200, Manfred Spraul wrote:
 Hi Davidlohr,
 
 On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
  The default size for shmmax is, and always has been, 32Mb.
  Today, in the XXI century, it seems that this value is rather small,
  making users have to increase it via sysctl, which can cause
  unnecessary work and userspace application workarounds[1].
 
  Instead of choosing yet another arbitrary value, larger than 32Mb,
  this patch disables the use of both shmmax and shmall by default,
  allowing users to create segments of unlimited sizes. Users and
  applications that already explicitly set these values through sysctl
  are left untouched, and thus does not change any of the behavior.
 
  So a value of 0 bytes or pages, for shmmax and shmall, respectively,
  implies unlimited memory, as opposed to disabling sysv shared memory.
  This is safe as 0 cannot possibly be used previously as SHMMIN is
  hardcoded to 1 and cannot be modified.

 Are we sure that no user space apps uses shmctl(IPC_INFO) and prints a 
 pretty error message if shmall is too small?
 We would break these apps.

Good point. 0 bytes/pages would definitely trigger an unexpected error
message if users did this. But on the other hand I'm not sure this
actually is a _real_ scenario, since upon overflow the value can still
end up being 0, which is totally bogus and would cause the same
breakage.

So I see two possible workarounds:
(i) Use ULONG_MAX for the shmmax default instead. This would make shmall
default to 1152921504606846720 and 268435456, for 64 and 32bit systems,
respectively.

(ii) Keep the 0 bytes, but add a new a transition tunable that, if set
(default off), would allow 0 bytes to be unlimited. With time, users
could hopefully update their applications and we could eventually get
rid of it. This _seems_ to be the less aggressive way to go.

Thoughts?

Thanks,
Davidlohr

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread KOSAKI Motohiro
On Wed, Apr 2, 2014 at 8:20 PM, Davidlohr Bueso davidl...@hp.com wrote:
 The default size for shmmax is, and always has been, 32Mb.
 Today, in the XXI century, it seems that this value is rather small,
 making users have to increase it via sysctl, which can cause
 unnecessary work and userspace application workarounds[1].

 Instead of choosing yet another arbitrary value, larger than 32Mb,
 this patch disables the use of both shmmax and shmall by default,
 allowing users to create segments of unlimited sizes. Users and
 applications that already explicitly set these values through sysctl
 are left untouched, and thus does not change any of the behavior.

 So a value of 0 bytes or pages, for shmmax and shmall, respectively,
 implies unlimited memory, as opposed to disabling sysv shared memory.
 This is safe as 0 cannot possibly be used previously as SHMMIN is
 hardcoded to 1 and cannot be modified.

 This change allows Linux to treat shm just as regular anonymous memory.
 One important difference between them, though, is handling out-of-memory
 conditions: as opposed to regular anon memory, the OOM killer will not
 kill processes that are hogging memory through shm, allowing users to
 potentially abuse this. To overcome this situation, the shm_rmid_forced
 option must be enabled.

I'm very slightly against this sentence.

OOM killer WILL kill the process because shm touching increase RSS anyway.
But the killing doesn't make memory freeing because it's shmem.


 Running this patch through LTP, everything passes, except the following,
 which, due to the nature of this change, is quite expected:

 shmget021  TFAIL  :  call succeeded unexpectedly

 [1]: http://rhaas.blogspot.com/2012/06/absurd-shared-memory-limits.html

 Signed-off-by: Davidlohr Bueso davidl...@hp.com
 ---
  include/linux/shm.h  | 2 +-
  include/uapi/linux/shm.h | 8 
  ipc/shm.c| 6 --
  3 files changed, 9 insertions(+), 7 deletions(-)

 diff --git a/include/linux/shm.h b/include/linux/shm.h
 index 1e2cd2e..0ca06a3 100644
 --- a/include/linux/shm.h
 +++ b/include/linux/shm.h
 @@ -4,7 +4,7 @@
  #include asm/page.h
  #include uapi/linux/shm.h

 -#define SHMALL (SHMMAX/PAGE_SIZE*(SHMMNI/16)) /* max shm system wide (pages) 
 */
 +#define SHMALL 0 /* max shm system wide (pages) */
  #include asm/shmparam.h
  struct shmid_kernel /* private to the kernel */
  {
 diff --git a/include/uapi/linux/shm.h b/include/uapi/linux/shm.h
 index 78b6941..5f0ef28 100644
 --- a/include/uapi/linux/shm.h
 +++ b/include/uapi/linux/shm.h
 @@ -9,14 +9,14 @@

  /*
   * SHMMAX, SHMMNI and SHMALL are upper limits are defaults which can
 - * be increased by sysctl
 + * be increased by sysctl. By default, disable SHMMAX and SHMALL with
 + * 0 bytes, thus allowing processes to have unlimited shared memory.
   */
 -
 -#define SHMMAX 0x200/* max shared seg size (bytes) */
 +#define SHMMAX 0/* 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))
 +#define SHMALL 0
  #endif
  #define SHMSEG SHMMNI   /* max shared segs per process */

 diff --git a/ipc/shm.c b/ipc/shm.c
 index 7645961..ae01ffa 100644
 --- a/ipc/shm.c
 +++ b/ipc/shm.c
 @@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct 
 ipc_params *params)
 int id;
 vm_flags_t acctflag = 0;

 -   if (size  SHMMIN || size  ns-shm_ctlmax)
 +   if (ns-shm_ctlmax 
 +   (size  SHMMIN || size  ns-shm_ctlmax))
 return -EINVAL;

 -   if (ns-shm_tot + numpages  ns-shm_ctlall)
 +   if (ns-shm_ctlall 
 +   ns-shm_tot + numpages  ns-shm_ctlall)
 return -ENOSPC;

 shp = ipc_rcu_alloc(sizeof(*shp));

Looks good.

Acked-by: KOSAKI Motohiro kosaki.motoh...@jp.fujitsu.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread KOSAKI Motohiro
On Thu, Apr 3, 2014 at 3:50 PM, Davidlohr Bueso davidl...@hp.com wrote:
 On Thu, 2014-04-03 at 21:02 +0200, Manfred Spraul wrote:
 Hi Davidlohr,

 On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
  The default size for shmmax is, and always has been, 32Mb.
  Today, in the XXI century, it seems that this value is rather small,
  making users have to increase it via sysctl, which can cause
  unnecessary work and userspace application workarounds[1].
 
  Instead of choosing yet another arbitrary value, larger than 32Mb,
  this patch disables the use of both shmmax and shmall by default,
  allowing users to create segments of unlimited sizes. Users and
  applications that already explicitly set these values through sysctl
  are left untouched, and thus does not change any of the behavior.
 
  So a value of 0 bytes or pages, for shmmax and shmall, respectively,
  implies unlimited memory, as opposed to disabling sysv shared memory.
  This is safe as 0 cannot possibly be used previously as SHMMIN is
  hardcoded to 1 and cannot be modified.

 Are we sure that no user space apps uses shmctl(IPC_INFO) and prints a
 pretty error message if shmall is too small?
 We would break these apps.

 Good point. 0 bytes/pages would definitely trigger an unexpected error
 message if users did this. But on the other hand I'm not sure this
 actually is a _real_ scenario, since upon overflow the value can still
 end up being 0, which is totally bogus and would cause the same
 breakage.

 So I see two possible workarounds:
 (i) Use ULONG_MAX for the shmmax default instead. This would make shmall
 default to 1152921504606846720 and 268435456, for 64 and 32bit systems,
 respectively.

 (ii) Keep the 0 bytes, but add a new a transition tunable that, if set
 (default off), would allow 0 bytes to be unlimited. With time, users
 could hopefully update their applications and we could eventually get
 rid of it. This _seems_ to be the less aggressive way to go.

Do you mean

set 0: IPC_INFO return shmmax = 0.
set 1: IPC_INFO return shmmax = ULONG_MAX.

?

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


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread KOSAKI Motohiro
 This change allows Linux to treat shm just as regular anonymous memory.
 One important difference between them, though, is handling out-of-memory
 conditions: as opposed to regular anon memory, the OOM killer will not
 kill processes that are hogging memory through shm, allowing users to
 potentially abuse this. To overcome this situation, the shm_rmid_forced
 option must be enabled.

Off topic: systemd implemented similar feature RemoveIPC and it is
enabled by default.
http://lists.freedesktop.org/archives/systemd-devel/2014-March/018232.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] ipc,shm: disable shmmax and shmall by default

2014-04-03 Thread Davidlohr Bueso
On Thu, 2014-04-03 at 19:39 -0400, KOSAKI Motohiro wrote:
 On Thu, Apr 3, 2014 at 3:50 PM, Davidlohr Bueso davidl...@hp.com wrote:
  On Thu, 2014-04-03 at 21:02 +0200, Manfred Spraul wrote:
  Hi Davidlohr,
 
  On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
   The default size for shmmax is, and always has been, 32Mb.
   Today, in the XXI century, it seems that this value is rather small,
   making users have to increase it via sysctl, which can cause
   unnecessary work and userspace application workarounds[1].
  
   Instead of choosing yet another arbitrary value, larger than 32Mb,
   this patch disables the use of both shmmax and shmall by default,
   allowing users to create segments of unlimited sizes. Users and
   applications that already explicitly set these values through sysctl
   are left untouched, and thus does not change any of the behavior.
  
   So a value of 0 bytes or pages, for shmmax and shmall, respectively,
   implies unlimited memory, as opposed to disabling sysv shared memory.
   This is safe as 0 cannot possibly be used previously as SHMMIN is
   hardcoded to 1 and cannot be modified.
 
  Are we sure that no user space apps uses shmctl(IPC_INFO) and prints a
  pretty error message if shmall is too small?
  We would break these apps.
 
  Good point. 0 bytes/pages would definitely trigger an unexpected error
  message if users did this. But on the other hand I'm not sure this
  actually is a _real_ scenario, since upon overflow the value can still
  end up being 0, which is totally bogus and would cause the same
  breakage.
 
  So I see two possible workarounds:
  (i) Use ULONG_MAX for the shmmax default instead. This would make shmall
  default to 1152921504606846720 and 268435456, for 64 and 32bit systems,
  respectively.
 
  (ii) Keep the 0 bytes, but add a new a transition tunable that, if set
  (default off), would allow 0 bytes to be unlimited. With time, users
  could hopefully update their applications and we could eventually get
  rid of it. This _seems_ to be the less aggressive way to go.
 
 Do you mean
 
 set 0: IPC_INFO return shmmax = 0.
 set 1: IPC_INFO return shmmax = ULONG_MAX.
 
 ?
 
 That makes sense.

Well I was mostly referring to:

set 0: leave things as there are now.
set 1: this patch.

I don't think it makes much sense to set unlimited for both 0 and
ULONG_MAX, that would probably just create even more confusion. 

But then again, we shouldn't even care about breaking things with shmmax
or shmall with 0 value, it just makes no sense from a user PoV. shmmax
cannot be 0 unless there's an overflow, which voids any valid cases, and
thus shmall cannot be 0 either as it would go against any values set for
shmmax. I think it's safe to ignore this.


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