linux-kernel - Re: [PATCH 2/6] rds: Brute force GFP

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]

Date: Mon, 13 May 2024 19:14:24 +0100
From: Simon Horman <horms@...nel.org>
To: Håkon Bugge <haakon.bugge@...cle.com>
Cc: linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, rds-devel@....oracle.com,
	Jason Gunthorpe <jgg@...pe.ca>, Leon Romanovsky <leon@...nel.org>,
	Saeed Mahameed <saeedm@...dia.com>,
	Tariq Toukan <tariqt@...dia.com>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Tejun Heo <tj@...nel.org>, Lai Jiangshan <jiangshanlai@...il.com>,
	Allison Henderson <allison.henderson@...cle.com>,
	Manjunath Patil <manjunath.b.patil@...cle.com>,
	Mark Zhang <markzhang@...dia.com>,
	Chuck Lever <chuck.lever@...cle.com>,
	Shiraz Saleem <shiraz.saleem@...el.com>,
	Yang Li <yang.lee@...ux.alibaba.com>
Subject: Re: [PATCH 2/6] rds: Brute force GFP_NOIO

On Mon, May 13, 2024 at 02:53:42PM +0200, Håkon Bugge wrote:
> For most entry points to RDS, we call memalloc_noio_{save,restore} in
> a parenthetic fashion when enabled by the module parameter force_noio.
> 
> We skip the calls to memalloc_noio_{save,restore} in rds_ioctl(), as
> no memory allocations are executed in this function or its callees.
> 
> The reason we execute memalloc_noio_{save,restore} in rds_poll(), is
> due to the following call chain:
> 
> rds_poll()
>         poll_wait()
>                 __pollwait()
>                         poll_get_entry()
>                                 __get_free_page(GFP_KERNEL)
> 
> The function rds_setsockopt() allocates memory in its callee's
> rds_get_mr() and rds_get_mr_for_dest(). Hence, we need
> memalloc_noio_{save,restore} in rds_setsockopt().
> 
> In rds_getsockopt(), we have rds_info_getsockopt() that allocates
> memory. Hence, we need memalloc_noio_{save,restore} in
> rds_getsockopt().
> 
> All the above, in order to conditionally enable RDS to become a block I/O
> device.
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@...cle.com>

Hi Håkon,

Some minor feedback from my side.

> ---
>  net/rds/af_rds.c | 60 +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 3 deletions(-)
>
> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index 8435a20968ef5..a89d192aabc0b 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c
> @@ -37,10 +37,16 @@                                                           >  #include <linux/in.h>
>  #include <linux/ipv6.h>
>  #include <linux/poll.h>
> +#include <linux/sched/mm.h>
>  #include <net/sock.h>
>
>  #include "rds.h"
>
> +bool rds_force_noio;
> +EXPORT_SYMBOL(rds_force_noio);

rds_force_noio seems to be only used within this file.
I wonder if it should it be static and not EXPORTed?

Flagged by Sparse.

> +module_param_named(force_noio, rds_force_noio, bool, 0444);
> +MODULE_PARM_DESC(force_noio, "Force the use of GFP_NOIO (Y/N)");
> +
>  /* this is just used for stats gathering :/ */
>  static DEFINE_SPINLOCK(rds_sock_lock);
>  static unsigned long rds_sock_count;
> @@ -60,6 +66,10 @@ static int rds_release(struct socket *sock)
>  {
>  	struct sock *sk = sock->sk;
>  	struct rds_sock *rs;
> +	unsigned int noio_flags;

Please consider using reverse xmas tree order - longest line to shortest -
for local variable declarations in Networking code.

This tool can be of assistance: https://github.com/ecree-solarflare/xmastree

> +
> +	if (rds_force_noio)
> +		noio_flags = memalloc_noio_save();
>  
>  	if (!sk)
>  		goto out;

..

> @@ -324,6 +346,8 @@ static int rds_cancel_sent_to(struct rds_sock *rs, sockptr_t optval, int len)
>  
>  	rds_send_drop_to(rs, &sin6);
>  out:
> +	if (rds_force_noio)
> +		noio_flags = memalloc_noio_save();

noio_flags appears to be set but otherwise unused in this function.

Flagged by W=1 builds.

>  	return ret;
>  }
>  

..