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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <470DC988.3020506@bull.net>
Date:	Thu, 11 Oct 2007 08:58:16 +0200
From:	Nadia Derbey <Nadia.Derbey@...l.net>
To:	Pierre Peiffer <Pierre.Peiffer@...l.net>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] IPC: fix error case when idr-cache is empty in ipcget()

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




-
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/