netdev - 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]

Message-ID: <564056C5-59A6-4C32-80E1-1E5DB6275989@oracle.com>
Date: Tue, 14 May 2024 13:31:43 +0000
From: Haakon Bugge <haakon.bugge@...cle.com>
To: Simon Horman <horms@...nel.org>
CC: OFED mailing list <linux-rdma@...r.kernel.org>,
        open list
	<linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        "rds-devel@....oracle.com" <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 III <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.

Hi Simon,

You are quite right. Had an earlier version where the symbol was used in several files, but in this version, static is the right choice. Fixed in v2.

> +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

Will fix.

> 
> +
> +	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.

Bummer. C/P error. This should be the restore. Fixed in v2. Will add W=1 for builds in the future :-)


Thxs, Håkon