[CRIU] [PATCH] cr-check: Inspect errno of prctl calls

[Pavel Emelyanov]

On 03/12/2016 01:07 AM, Filipe Brandenburger wrote:
> When it fails, the prctl() syscall will return -1 and set `errno`
> appropriately (instead of returning the error number code directly), so
> we should check for ret < 0 and inspect errno instead of comparing the
> return value with the error codes.
> 
> This bug was introduced in commit 8ceab588a5ce91 ("crtools: no more
> linked with builtin syscall") which replaced sys_prctl() (which returned
> the error number code directly) with prctl() (which returns -1 and sets
> the `errno` variable.)
> 
> Also make all comparisons of "ret" explicit to check < 0 instead of the
> implicit != 0 (PR_SET_MM is supposed to only return 0 or -1, but it's
> good to be explicit in case this code is later refactored and that check
> is not updated.)
> 
> Tested that now `criu check --ms` properly reports "prctl: PR_SET_MM is
> not supported" (when indeed not supported) instead of the misleading
> "prctl: One needs CAP_SYS_RESOURCE capability to perform testing."
> 
> Reported-by: Saied Kazemi <saied at google.com>
> Cc: Laurent Dufour <ldufour at linux.vnet.ibm.com>
> Signed-off-by: Filipe Brandenburger <filbranden at google.com>

Ah :) Filipe was faster than me fixing the errno-s for prctls!

Please, find my comments inline.

> ---
>  criu/cr-check.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/criu/cr-check.c b/criu/cr-check.c
> index 2896b0280924..63e7197f5f65 100644
> --- a/criu/cr-check.c
> +++ b/criu/cr-check.c
> @@ -177,7 +177,7 @@ static int check_prctl(void)
>  	int ret;
>  
>  	ret = prctl(PR_GET_TID_ADDRESS, (unsigned long)&tid_addr, 0, 0, 0);
> -	if (ret) {
> +	if (ret < 0) {
>  		pr_msg("prctl: PR_GET_TID_ADDRESS is not supported");
>  		return -1;
>  	}
> @@ -186,7 +186,7 @@ static int check_prctl(void)
>  	 * Either new or old interface must be supported in the kernel.
>  	 */
>  	ret = prctl(PR_SET_MM, PR_SET_MM_MAP_SIZE, (unsigned long)&size, 0, 0);
> -	if (ret) {
> +	if (ret < 0) {
>  		if (!opts.check_ms_kernel) {
>  			pr_msg("prctl: PR_SET_MM_MAP is not supported, which "
>  			       "is required for restoring user namespaces\n");
> @@ -195,8 +195,8 @@ static int check_prctl(void)
>  			pr_warn("Skipping unssuported PR_SET_MM_MAP\n");
>  
>  		ret = prctl(PR_SET_MM, PR_SET_MM_BRK, brk(0), 0, 0);
> -		if (ret) {
> -			if (ret == -EPERM)
> +		if (ret < 0) {
> +			if (errno == -EPERM)

errno-s contain positive codes.

>  				pr_msg("prctl: One needs CAP_SYS_RESOURCE capability to perform testing\n");
>  			else
>  				pr_msg("prctl: PR_SET_MM is not supported\n");
> @@ -204,13 +204,13 @@ static int check_prctl(void)
>  		}
>  
>  		ret = prctl(PR_SET_MM, PR_SET_MM_EXE_FILE, -1, 0, 0);
> -		if (ret != -EBADF) {
> +		if (ret < 0 && errno != -EBADF) {

Same here.

>  			pr_msg("prctl: PR_SET_MM_EXE_FILE is not supported (%d)\n", ret);

Printing ret would be not informative, please, print errno instead.

>  			return -1;
>  		}
>  
>  		ret = prctl(PR_SET_MM, PR_SET_MM_AUXV, (long)&user_auxv, sizeof(user_auxv), 0);
> -		if (ret) {
> +		if (ret < 0) {
>  			pr_msg("prctl: PR_SET_MM_AUXV is not supported\n");
>  			return -1;
>  		}
>