Re: [PATCH net] ethtool: vclock: propagate errors from ptp_get_vclocks_index()

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jakub Kicinski wrote:
> ptp_get_vclocks_index() was written with sock_timestamping_bind_phc()
> in mind. Any return <= 0 will get converted to EINVAL by the virtue
> of sock_timestamping_bind_phc() not finding a matching vclock.
> 
> ethtool's phc_vclocks_prepare_data(), however, should not be silently
> returning empty messages on allocation errors or when PHC does not
> exist at all.
> 
> Commit c156174a6707 ("ethtool: add a new command for getting PHC virtual
> clocks") added phc_vclocks_prepare_data(), but let's not treat this
> as a fix in case some user space app now depends on the failures being
> silent.
> 
> The check in sock_timestamping_bind_phc() is not strickly necessary,

strictly

> but include it just for clarity.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Willem de Bruijn <willemb@google.com>

>  
> diff --git a/net/core/sock.c b/net/core/sock.c
> index b37b664b6eb9..9e33e979a283 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -870,6 +870,8 @@ static int sock_timestamping_bind_phc(struct sock *sk, int phc_index)
>  
>  	num = ethtool_get_phc_vclocks(dev, &vclock_index);
>  	dev_put(dev);
> +	if (num < 0)
> +		return num;
>  
>  	for (i = 0; i < num; i++) {
>  		if (*(vclock_index + i) == phc_index) {

Not for this patch:

This is called by sk_setsockopt and by MPTCP for every subflow.
Apparently MPTCP does not check the return value. Should it?