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:
> >>>
> >>> 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.
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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>