On Tue, 2013-12-03 at 10:44 +0100, Petr Mladek wrote:
> When trying to understand semop code, I found a small mistake in the check for
> semadj (undo) value overflow. The new undo value is not stored immediately
> and next potential checks are done against the old value.
>
> The failing scenario is not much practical. One semop call has to do more
> operations on the same semaphore. Also semval and semadj must have different
> values, so there has to be some operations without SEM_UNDO flag. For example:
>
> struct sembuf depositor_op[1];
> struct sembuf collector_op[2];
>
> depositor_op[0].sem_num = 0;
> depositor_op[0].sem_op = 20000;
> depositor_op[0].sem_flg = 0;
>
> collector_op[0].sem_num = 0;
> collector_op[0].sem_op = -10000;
> collector_op[0].sem_flg = SEM_UNDO;
> collector_op[1].sem_num = 0;
> collector_op[1].sem_op = -10000;
> collector_op[1].sem_flg = SEM_UNDO;
>
> if (semop(semid, depositor_op, 1) == -1)
> { perror("Failed to do 1st deposit"); return 1; }
>
> if (semop(semid, collector_op, 2) == -1)
> { perror("Failed to do 1st collect"); return 1; }
>
> if (semop(semid, depositor_op, 1) == -1)
> { perror("Failed to do 2nd deposit"); return 1; }
>
> if (semop(semid, collector_op, 2) == -1)
> { perror("Failed to do 2nd collect"); return 1; }
>
> return 0;
>
> It passes without error now but the semadj value has overflown in the
> 2nd collector operation.
>
> Signed-off-by: Petr Mladek <pmla...@suse.cz>
Seems sane.
Acked-by: Davidlohr Bueso <davidl...@hp.com>
> ---
> ipc/sem.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/ipc/sem.c b/ipc/sem.c
> index db9d241af133..0d4375761449 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -599,7 +599,7 @@ SYSCALL_DEFINE3(semget, key_t, key, int, nsems, int,
> semflg)
> static int perform_atomic_semop(struct sem_array *sma, struct sembuf *sops,
> int nsops, struct sem_undo *un, int pid)
> {
> - int result, sem_op;
> + int result, undo, sem_op;
> struct sembuf *sop;
> struct sem * curr;
>
> @@ -607,7 +607,7 @@ static int perform_atomic_semop(struct sem_array *sma,
> struct sembuf *sops,
> curr = sma->sem_base + sop->sem_num;
> sem_op = sop->sem_op;
> result = curr->semval;
> -
> +
> if (!sem_op && result)
> goto would_block;
>
> @@ -616,25 +616,24 @@ static int perform_atomic_semop(struct sem_array *sma,
> struct sembuf *sops,
> goto would_block;
> if (result > SEMVMX)
> goto out_of_range;
> +
> if (sop->sem_flg & SEM_UNDO) {
> - int undo = un->semadj[sop->sem_num] - sem_op;
> - /*
> - * Exceeding the undo range is an error.
> - */
> + undo = un->semadj[sop->sem_num] - sem_op;
> + /* Exceeding the undo range is an error. */
> if (undo < (-SEMAEM - 1) || undo > SEMAEM)
> goto out_of_range;
> + un->semadj[sop->sem_num] = undo;
> }
> +
> curr->semval = result;
> }
>
> sop--;
> while (sop >= sops) {
> sma->sem_base[sop->sem_num].sempid = pid;
> - if (sop->sem_flg & SEM_UNDO)
> - un->semadj[sop->sem_num] -= sop->sem_op;
> sop--;
> }
> -
> +
> return 0;
>
> out_of_range:
> @@ -650,7 +649,10 @@ would_block:
> undo:
> sop--;
> while (sop >= sops) {
> - sma->sem_base[sop->sem_num].semval -= sop->sem_op;
> + sem_op = sop->sem_op;
> + sma->sem_base[sop->sem_num].semval -= sem_op;
> + if (sop->sem_flg & SEM_UNDO)
> + un->semadj[sop->sem_num] += sem_op;
> sop--;
> }
>
--
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/
