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  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]
Date:   Mon, 14 Dec 2020 09:35:42 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Tariq Toukan <tariqt@...dia.com>
Cc:     "John W. Linville" <linville@...driver.com>,
        netdev@...r.kernel.org, Roy Novich <royno@...dia.com>
Subject: Re: [PATCH ethtool] ethtool: do_sset return correct value on fail

On Sun, Dec 13, 2020 at 04:25:03PM +0200, Tariq Toukan wrote:
> From: Roy Novich <royno@...dia.com>
> 
> The return value for do_sset was constant and returned 0.
> This value is misleading when returned on operation failure.
> Changed return value to the correct function err status.
> 
> Fixes: 32c8037055f5 ("Initial import of ethtool version 3 + a few patches.")
> Signed-off-by: Roy Novich <royno@...dia.com>
> Signed-off-by: Tariq Toukan <tariqt@...dia.com>
> ---
>  ethtool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 1d9067e774af..5cc875c64591 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -3287,7 +3287,7 @@ static int do_sset(struct cmd_context *ctx)
>  		}
>  	}
>  
> -	return 0;
> +	return err;
>  }
>  
>  static int do_gregs(struct cmd_context *ctx)

I'm afraid it's not as easy as this. The problem with -s/--set
subcommand is that its parameters are in fact implemented by three
separate requests (for ioctl, four requests for netlink). Each of them
may fail or succeed independently of others. Currently do_sset() always
returns 0 which is indeed unfortunate but it's not clear what should we
return if some requests fail and some succeed.

With your patch, do_sset() would always return the result of last
request it sent to kernel which is inconsistent; consider e.g.

  ethtool -s eth0 mdix on wol g

If the ETHTOOL_SLINKSETTINGS request setting MDI-X succeeds and
ETHTOOL_SWOL setting WoL fails, the result would be a failure. But if
setting MDI-X fails and setting WoL succeeds, do_sset() would report
success.

So if we really want to change the behaviour after so many years, it
should be consistent: either return non-zero exit code if any request
fails or if all fail. (Personally, I would prefer the latter.) And we
should probably modify nl_sset() to behave the same way as it currently
bails out on first failed request.

Michal

Powered by blists - more mailing lists