netdev - Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

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: <20250304095036.00000edf@huawei.com>
Date: Tue, 4 Mar 2025 09:50:36 +0800
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: Dave Jiang <dave.jiang@...el.com>, Shannon Nelson
	<shannon.nelson@....com>, <andrew.gospodarek@...adcom.com>,
	<aron.silverton@...cle.com>, <dan.j.williams@...el.com>,
	<daniel.vetter@...ll.ch>, <dsahern@...nel.org>, <gregkh@...uxfoundation.org>,
	<hch@...radead.org>, <itayavr@...dia.com>, <jiri@...dia.com>,
	<kuba@...nel.org>, <lbloch@...dia.com>, <leonro@...dia.com>,
	<linux-cxl@...r.kernel.org>, <linux-rdma@...r.kernel.org>,
	<netdev@...r.kernel.org>, <saeedm@...dia.com>, <brett.creeley@....com>
Subject: Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

On Mon, 3 Mar 2025 13:29:53 -0400
Jason Gunthorpe <jgg@...dia.com> wrote:

> On Mon, Mar 03, 2025 at 09:45:52AM -0700, Dave Jiang wrote:
> > > +static int pdsfc_probe(struct auxiliary_device *adev,
> > > +		       const struct auxiliary_device_id *id)
> > > +{
> > > +	struct pds_auxiliary_dev *padev =
> > > +			container_of(adev, struct pds_auxiliary_dev, aux_dev);
> > > +	struct pdsfc_dev *pdsfc __free(pdsfc_dev) =
> > > +			fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
> > > +					   struct pdsfc_dev, fwctl);
> > > +	struct device *dev = &adev->dev;
> > > +	int err;
> > > +  
> > 
> > It's ok to move the pdsfc declaration inline right before this check
> > below. That would help prevent any accidental usages of pdsfc before
> > the check. This is an exception to the coding style guidelines with
> > the introduction of cleanup mechanisms.  
> 
> Yeah.. I'm starting to feel negative about cleanup.h - there are too
> many special style notes that seem to only be known by hearsay :\

I think this one is more about the classic risk of undefined
behavior meaning the compiler removes the check.  So if we had a
bob = pdsfc->fred;

but didn't use bob before the check on pdsfc != NULL then
the compiler is allowed to remove the check on pdsfc existing
as the dereferencce can be assume to mean pdsfc is always
!= NULL.

So not really a cleanup.h related issue at all (and not in my
view suitable for adding to the nice help Dan put in that file).

Doesn't matter too much though.  Personally I'm kind of content
now with cleanup.h usage but it was a bit of time to teach
myself the new way of thinking about things.

Jonathan


> 
> > > +static void pdsfc_remove(struct auxiliary_device *adev)
> > > +{
> > > +	struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
> > > +
> > > +	fwctl_unregister(&pdsfc->fwctl);  
> > 
> > Missing fwctl_put(). See fwctl_unregister() header comments. Caller
> > must still call fwctl_put() to free the fwctl after calling
> > fwctl_unregister().  
> 
> The code is correct, the put is hidden in the __free
> 
> However I think we decided not to use __free in this context and open
> code the put as a style choice
> 
> Thanks,
> Jason