netdev - Re: [PATCH v2 5/8] fwctl: FWCTL_RPC to execute a Remote Procedure Call to device firmware

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240729162818.GC3625856@nvidia.com>
Date: Mon, 29 Jul 2024 13:28:18 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Jonathan Corbet <corbet@....net>, Itay Avraham <itayavr@...dia.com>,
	Jakub Kicinski <kuba@...nel.org>, Leon Romanovsky <leon@...nel.org>,
	linux-doc@...r.kernel.org, linux-rdma@...r.kernel.org,
	netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
	Saeed Mahameed <saeedm@...dia.com>,
	Tariq Toukan <tariqt@...dia.com>,
	Andy Gospodarek <andrew.gospodarek@...adcom.com>,
	Aron Silverton <aron.silverton@...cle.com>,
	Dan Williams <dan.j.williams@...el.com>,
	David Ahern <dsahern@...nel.org>,
	Christoph Hellwig <hch@...radead.org>, Jiri Pirko <jiri@...dia.com>,
	Leonid Bloch <lbloch@...dia.com>,
	Leon Romanovsky <leonro@...dia.com>, linux-cxl@...r.kernel.org,
	patches@...ts.linux.dev
Subject: Re: [PATCH v2 5/8] fwctl: FWCTL_RPC to execute a Remote Procedure
 Call to device firmware

On Fri, Jul 26, 2024 at 04:30:09PM +0100, Jonathan Cameron wrote:
> > @@ -8,16 +8,20 @@
> >  #include <linux/slab.h>
> >  #include <linux/container_of.h>
> >  #include <linux/fs.h>
> > +#include <linux/sizes.h>
> >  
> >  #include <uapi/fwctl/fwctl.h>
> >  
> >  enum {
> >  	FWCTL_MAX_DEVICES = 256,
> > +	MAX_RPC_LEN = SZ_2M,
> >  };
> 
> In what way is that usefully handled as an enum?
> I'd just use #defines

I generally am not so keen on defines for constants.. There is some
advantage with clangd and gdb, for instance. Enum is the only other
option even though it is a bit of abuse to use it like this.

> >  DEFINE_FREE(kfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T));
> > +DEFINE_FREE(kvfree_errptr, void *, if (!IS_ERR_OR_NULL(_T)) kvfree(_T));
> kvfree define free already defined as this since 6.9

Ok
 
> > +	void *inbuf __free(kvfree) =
> > +		kvzalloc(cmd->in_len, GFP_KERNEL | GFP_KERNEL_ACCOUNT);
> 
> As before
> #define GFP_KERNEL_ACCOUNT (GFP_KERNEL | __GFP_ACCOUNT)
> so don't need both.

Yep
 
> > +	if (outbuf == inbuf) {
> > +		/* The driver can re-use inbuf as outbuf */
> > +		inbuf = NULL;
> I wish no_free_ptr() didn't have __must_check. Can do something ugly
> like
> 		outbuf = no_free_ptr(inbuf);
> probably but maybe just setting it NULL is simpler.

Yeah NULL seems clearer, the outbuf assignment is a bit too odd, IMHO

> > +	/**
> > +	 * @FWCTL_RPC_DEBUG_READ_ONLY: Read only access to debug information
> > +	 *
> > +	 * Readable debug information. Debug information is compatible with
> > +	 * kernel lockdown, and does not disclose any sensitive information. For
> > +	 * instance exposing any encryption secrets from this information is
> > +	 * forbidden.
> > +	 */
> > +	FWCTL_RPC_DEBUG_READ_ONLY = 1,
> > +	/**
> > +	 * @FWCTL_RPC_DEBUG_WRITE: Writable access to lockdown compatible debug information
> 
> Write access
> probably rather than writeable.

Sure

Thanks,
Jason