linux-kernel - Re: [RFC] PTR_ERR: return 0 if ptr isn't an error value.

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130603071526.GA5483@pengutronix.de>
Date:	Mon, 3 Jun 2013 09:15:26 +0200
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	Thomas Meyer <thomas@...3r.de>, mst@...hat.com,
	grant.likely@...aro.org, rob.herring@...xeda.com,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Julia Lawall <Julia.Lawall@...6.fr>
Subject: Re: [RFC] PTR_ERR: return 0 if ptr isn't an error value.

Hello Rusty,

[added akpm to Cc: who took the patch back then and Julia for the
coccinelle part below]

On Mon, Jun 03, 2013 at 11:59:15AM +0930, Rusty Russell wrote:
> 
> Back in 2011, Uwe Kleine-König added the nonsensically-named
> PTR_RET(), providing a means to avoid if() statements in code (commit 
> fa9ee9c4b9).
> 
> Instead, just make PTR_ERR() return 0 if the pointer isn't an error
> value.  This is harmless, since PTR_ERR() should have never been
> passed a non-error value.  And GCC is usually smart enough to remove
> the extra test if IS_ERR() has already been called.
I wonder in which situations gcc fails to be smart enough. Did you check
this?

> My vmlinux text increased by 300 bytes:
> 
> 	   text	   data	    bss	    dec	    hex	filename
> 	6029452	 491628	2576384	9097464	 8ad0f8	vmlinux
> 	6029721	 491628	2576384	9097733	 8ad205	vmlinux.PTR_ERR
> 
> Cc: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> Cc: Thomas Meyer <thomas@...3r.de>
> Signed-off-by: Rusty Russell <rusty@...tcorp.com.au>
> 
> diff --git a/include/linux/err.h b/include/linux/err.h
> index f2edce2..621d859 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -24,14 +24,16 @@ static inline void * __must_check ERR_PTR(long error)
>  	return (void *) error;
>  }
>  
> -static inline long __must_check PTR_ERR(const void *ptr)
> +static inline long __must_check IS_ERR(const void *ptr)
>  {
> -	return (long) ptr;
> +	return IS_ERR_VALUE((unsigned long)ptr);
>  }
>  
> -static inline long __must_check IS_ERR(const void *ptr)
> +static inline long __must_check PTR_ERR(const void *ptr)
>  {
> -	return IS_ERR_VALUE((unsigned long)ptr);
> +	if (IS_ERR(ptr))
> +		return (long) ptr;
> +	return 0;
>  }
>  
>  static inline long __must_check IS_ERR_OR_NULL(const void *ptr)
> @@ -52,14 +54,7 @@ static inline void * __must_check ERR_CAST(const void *ptr)
>  	return (void *) ptr;
>  }
>  
> -static inline int __must_check PTR_RET(const void *ptr)
> -{
> -	if (IS_ERR(ptr))
> -		return PTR_ERR(ptr);
> -	else
> -		return 0;
> -}
> -
> +#define PTR_RET	PTR_ERR
I'd add a comment here that PTR_RET shouldn't be used anymore.

>  #endif
>  
>  #endif /* _LINUX_ERR_H */
> 

Is it worth to apply the same change to tools/virtio/linux/err.h to
minimize the chance for later surprises?
Also scripts/coccinelle/api/ptr_ret.cocci starts giving false warnings.

Other than that I think the change is fine.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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/