Re: [PATCH] ipc,shm: disable shmmax and shmall by default
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
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
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
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
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
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,&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
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,&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
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
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
> 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
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
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
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
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 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/
