| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 08 Aug 2012 14:53:58 +0900
From: Seiichi Ikarashi <s.ikarashi@...fujitsu.com>
To: Manfred Spraul <manfred@...orfullife.com>
CC: linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ipc/sem.c: prevent ENOMEM in semop() w/ SEM_UNDO flag
Hi Manfred,
(2012-08-07 20:10), Manfred Spraul wrote:
> Hi Seiichi,
>
> 2012/8/6 Seiichi Ikarashi <s.ikarashi@...fujitsu.com>
>>
>>
>> A real case was as follows.
>> semget(IPC_PRIVATE, 70000, IPC_CREAT | IPC_EXCL);
>> sops[0].sem_num = 0;
>> sops[0].sem_op = 1;
>> sops[0].sem_flg = SEM_UNDO;
>> semop(semid, sops, 1);
>>
>
> I think this can't work: sops[].sem_num is defined as "unsigned short".
> Thus more than 65500 semaphores in one semaphore set do not make
> any sense.
> "unsigned short" is also specified in the opengroup standard:
>
> http://pubs.opengroup.org/onlinepubs/7908799/xsh/syssem.h.html
>
> Thus: The hard limit is 65535. Perhaps slightly less, I haven't checked
> if (-1) is used somewhere to indicate an error.
Oops, you are correct.
More than 65536 semaphores in one set do not make sense
according to the definition.
>
> Is it possible to split the semaphores into multiple semphore ids?
> e.g. 70 ids, each with 1000 semaphores?
>
> The atomicity would be lost (e.g. all SEM_UNDO operations within
> one id are performed at once. With 70 ids, the SEM_UNDOs are not
> atomic anymore)
Thank you for your kind suggestion.
>
>>
>> #define SEMMSL 250 /* <= 8 000 max num of semaphores per id */
>>
>
> As far as I can see your patch removes the last part of the code that
> caused the restriction to 8.000 semaphores per id.
Unfortunately no. My previous patch modified only the allocation part
and ignored the free part. Now I think the patch should be like this;
--- a/ipc/sem.c 2012-08-03 16:52:01.000000000 +0900
+++ b/ipc/sem.c 2012-08-08 14:16:11.000000000 +0900
@@ -735,6 +735,11 @@ static int count_semzcnt (struct sem_arr
return semzcnt;
}
+static void vfree_rcu( , )
+{
+ // something like call_rcu()
+}
+
/* Free a semaphore set. freeary() is called with sem_ids.rw_mutex locked
* as a writer and the spinlock for this semaphore set hold. sem_ids.rw_mutex
* remains locked on exit.
@@ -754,7 +759,7 @@ static void freeary(struct ipc_namespace
un->semid = -1;
list_del_rcu(&un->list_proc);
spin_unlock(&un->ulp->lock);
- kfree_rcu(un, rcu);
+ vfree_rcu(un, rcu);
}
/* Wake up all pending processes and let them fail with EIDRM. */
@@ -1258,17 +1263,18 @@ static struct sem_undo *find_alloc_undo(
sem_getref_and_unlock(sma);
/* step 2: allocate new undo structure */
- new = kzalloc(sizeof(struct sem_undo) + sizeof(short)*nsems, GFP_KERNEL);
+ new = ipc_alloc(sizeof(struct sem_undo) + sizeof(short)*nsems, GFP_KERNEL);
if (!new) {
sem_putref(sma);
return ERR_PTR(-ENOMEM);
}
+ memset(new, 0, sizeof(struct sem_undo) + sizeof(short)*nsems);
/* step 3: Acquire the lock on semaphore array */
sem_lock_and_putref(sma);
if (sma->sem_perm.deleted) {
sem_unlock(sma);
- kfree(new);
+ ipc_free(new);
un = ERR_PTR(-EIDRM);
goto out;
}
@@ -1279,7 +1285,7 @@ static struct sem_undo *find_alloc_undo(
*/
un = lookup_undo(ulp, semid);
if (un) {
- kfree(new);
+ ipc_free(new);
goto success;
}
/* step 5: initialize & link new undo structure */
@@ -1348,7 +1354,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid,
if (nsops > ns->sc_semopm)
return -E2BIG;
if(nsops > SEMOPM_FAST) {
- sops = kmalloc(sizeof(*sops)*nsops,GFP_KERNEL);
+ sops = ipc_alloc(sizeof(*sops)*nsops,GFP_KERNEL);
if(sops==NULL)
return -ENOMEM;
}
@@ -1541,7 +1547,7 @@ out_unlock_free:
wake_up_sem_queue_do(&tasks);
out_free:
if(sops != fast_sops)
- kfree(sops);
+ ipc_free(sops);
return error;
}
@@ -1669,7 +1675,7 @@ void exit_sem(struct task_struct *tsk)
sem_unlock(sma);
wake_up_sem_queue_do(&tasks);
- kfree_rcu(un, rcu);
+ vfree_rcu(un, rcu);
}
kfree(ulp);
}
I think I need a replacement of kfree_rcu() here, something like vfree_rcu().
I'm reading kernel/rcu* files now...
>
> Thus I'd propose that your patch changes this line to
>
> + #define SEMMSL 250 /* <= 65 500 max num of semaphores per id */
Sure, when above rcu matter is solved.
>
> And:
> I would add a comment into the patch description all semaphores
> from one id share a single kernel spinlock. This could be changed, but
Are you mentioning struct sem_array.sem_perm.lock?
> it would
> a) add complexity for all users and
> b) change user space visible behavior
> Thus I would prefer to avoid to implement it unless there are real
> applications that need this implementation.
Regards,
Seiichi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists