netdev - Re: [PATCH net-next 2/8] net/rds: Use max() to simplify the code

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: <20240828135310.GC1368797@kernel.org>
Date: Wed, 28 Aug 2024 14:53:10 +0100
From: Simon Horman <horms@...nel.org>
To: Hongbo Li <lihongbo22@...wei.com>
Cc: johannes@...solutions.net, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, allison.henderson@...cle.com,
	dsahern@...nel.org, pshelar@....org, linux-wireless@...r.kernel.org,
	netdev@...r.kernel.org, rds-devel@....oracle.com,
	dccp@...r.kernel.org, dev@...nvswitch.org,
	linux-afs@...ts.infradead.org
Subject: Re: [PATCH net-next 2/8] net/rds: Use max() to simplify the code

On Sat, Aug 24, 2024 at 03:40:27PM +0800, Hongbo Li wrote:
> The target if-else can be replaced with max().
> 
> Signed-off-by: Hongbo Li <lihongbo22@...wei.com>
> ---
>  net/rds/info.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/net/rds/info.c b/net/rds/info.c
> index b6b46a8214a0..8558b0a466b4 100644
> --- a/net/rds/info.c
> +++ b/net/rds/info.c
> @@ -194,10 +194,7 @@ int rds_info_getsockopt(struct socket *sock, int optname, char __user *optval,
>  	}
>  	ret = pin_user_pages_fast(start, nr_pages, FOLL_WRITE, pages);
>  	if (ret != nr_pages) {
> -		if (ret > 0)
> -			nr_pages = ret;
> -		else
> -			nr_pages = 0;
> +		nr_pages = max(ret, 0);

Along the same lines as Johannes Berg's comment on a different patch [1]
I think that there is a subtle but important difference, semantically,
between max() and that the existing code does, for which the best
description I can think of is setting a floor on the value.

Other than Johannes's comment, and now mine here, I think you will find
that, if you search the netdev ML, you will find this point being made
consistently, at least over the past year.

And yes, we understand that mathematically max() is doing the right thing.
But that is not the point that is being made here.

I suggest dropping this patch.
And any others like it.

[1] https://lore.kernel.org/all/d5f495b67fe6bf128e7a51b9fcfe11f70c9b66ae.camel@sipsolutions.net/

>  		ret = -EAGAIN; /* XXX ? */
>  		goto out;
>  	}
> -- 
> 2.34.1
> 
>