Re: [PATCH] IPC: fix error case when idr-cache is empty in ipcget()

Nadia Derbey
2007-10-11T06:52:25Z

Pierre Peiffer wrote:
> With the use of idr to store the ipc, the case where the idr cache is
> empty, when idr_get_new is called (this may happen even if we call
> idr_pre_get() before), is not well handled: it lets semget()/shmget()/msgget()
> return ENOSPC when this cache is empty, what 1. does not reflect the facts
> and 2. does not conform to the man(s).
>
> This patch fixes this by retrying the whole process of allocation in this case.
>
> Note: we could directly return ENOMEM if idr_pre_get() fails, but it does not
> mean that the cache is empty...
>

Pierre,

I agree with everythying, but not on idr_pre_get() failure: I think we
should give up in that case: the tests on idr_pre_get() return code
should remain IMHO (see after).

><snip>
>
> @@ -309,17 +309,20 @@ int ipc_addid(struct ipc_ids* ids, struc
> int ipcget_new(struct ipc_namespace *ns, struct ipc_ids *ids,
> struct ipc_ops *ops, struct ipc_params *params)
> {
> - int err;
> -
> - err = idr_pre_get(&ids->ipcs_idr, GFP_KERNEL);
> -
> - if (!err)
> - return -ENOMEM;
> + int err, alloc;
> +retry:
> + alloc = idr_pre_get(&ids->ipcs_idr, GFP_KERNEL);
>
> mutex_lock(&ids->mutex);
> err = ops->getnew(ns, params);
> mutex_unlock(&ids->mutex);
>
> + if (err == -EAGAIN) {
> + if (alloc)
> + goto retry;
> + else
> + err = -ENOMEM;
> + }
> return err;
> }

ops->getnew will call idr_get_new() that should not be called if
idr_pre_get() has failed. So I think that we should give up if
idr_pre_get() fails and the test on idr_pre_get() return code should not
be removed.

>
> @@ -372,9 +375,9 @@ int ipcget_public(struct ipc_namespace *
> {
> struct kern_ipc_perm *ipcp;
> int flg = params->flg;
> - int err;
> -
> - err = idr_pre_get(&ids->ipcs_idr, GFP_KERNEL);
> + int err, alloc;
> +retry:
> + alloc = idr_pre_get(&ids->ipcs_idr, GFP_KERNEL);
>
> mutex_lock(&ids->mutex);
> ipcp = ipc_findkey(ids, params->key);
> @@ -382,8 +385,6 @@ int ipcget_public(struct ipc_namespace *
> /* key not used */
> if (!(flg & IPC_CREAT))
> err = -ENOENT;
> - else if (!err)
> - err = -ENOMEM;
> else
> err = ops->getnew(ns, params);

Same remark as above:
ops->getnew will call idr_get_new() that should not be called if
idr_pre_get() has failed. So I think that we should give up if
idr_pre_get() fails and this this test should not be removed.

Regards,
Nadia