Messages in this thread | | | Date | Sat, 12 Apr 2014 10:50:11 +0200 | From | Manfred Spraul <> | Subject | 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: >>> >>> shmget02 1 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
| |