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

[Nadia Derbey <Nadia.Derbey@bull.net>]

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