linux-kernel - Re: [PATCH v2 13/23] counter: Provide alternative counter registration functions

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]

Date:   Wed, 29 Dec 2021 12:27:32 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Lars-Peter Clausen <lars@...afoo.de>, linux-iio@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        William Breathitt Gray <vilhelm.gray@...il.com>,
        linux-kernel@...r.kernel.org, kernel@...gutronix.de,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v2 13/23] counter: Provide alternative counter
 registration functions

Hello,

On Tue, Dec 28, 2021 at 06:00:17PM +0000, Jonathan Cameron wrote:
> On Mon, 27 Dec 2021 10:45:16 +0100
> Uwe Kleine-König         <u.kleine-koenig@...gutronix.de> wrote:
> 
> > +struct counter_device_allochelper {
> > +	struct counter_device counter;
> > +	unsigned long privdata[];
> Nice to keep this opaque. We danced around how to do this in IIO for
> a while and never got to a solution we all liked.  Slight disadvantage
> is this might matter in hot paths where the compiler doesn't have visibility
> to know it can very cheaply access the privdata.
> 
> I guess that can be looked at if anyone ever measures it as an important
> element (they probably never will!)

*nod*

> > +};
> > +
> >  static void counter_device_release(struct device *dev)
> >  {
> >  	struct counter_device *const counter =
> > @@ -31,6 +37,9 @@ static void counter_device_release(struct device *dev)
> >  
> >  	counter_chrdev_remove(counter);
> >  	ida_free(&counter_ida, dev->id);
> > +
> > +	if (!counter->legacy_device)
> > +		kfree(container_of(counter, struct counter_device_allochelper, counter));
> >  }
> >  
> >  static struct device_type counter_device_type = {
> > @@ -53,7 +62,14 @@ static dev_t counter_devt;
> >   */
> >  void *counter_priv(const struct counter_device *const counter)
> >  {
> > -	return counter->priv;
> > +	if (counter->legacy_device) {
> > +		return counter->priv;
> > +	} else {
> > +		struct counter_device_allochelper *ch =
> > +			container_of(counter, struct counter_device_allochelper, counter);
> > +
> > +		return &ch->privdata;
> > +	}
> >  }
> >  EXPORT_SYMBOL_GPL(counter_priv);
> >  
> > @@ -74,6 +90,8 @@ int counter_register(struct counter_device *const counter)
> >  	int id;
> >  	int err;
> >  
> > +	counter->legacy_device = true;
> > +
> >  	/* Acquire unique ID */
> >  	id = ida_alloc(&counter_ida, GFP_KERNEL);
> >  	if (id < 0)
> > @@ -114,6 +132,100 @@ int counter_register(struct counter_device *const counter)
> >  }
> >  EXPORT_SYMBOL_GPL(counter_register);
> >  
> > +/**
> > + * counter_alloc - allocate a counter_device
> > + * @sizeof_priv: size of the driver private data
> > + *
> > + * This is part one of counter registration. The structure is allocated
> > + * dynamically to ensure the right lifetime for the embedded struct device.
> > + *
> > + * If this succeeds, call counter_put() to get rid of the counter_device again.
> > + */
> > +struct counter_device *counter_alloc(size_t sizeof_priv)
> > +{
> > +	struct counter_device_allochelper *ch;
> > +	struct counter_device *counter;
> > +	struct device *dev;
> > +	int id, err;
> > +
> > +	ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL);
> > +	if (!ch) {
> > +		err = -ENOMEM;
> > +		goto err_alloc_ch;
> > +	}
> > +
> > +	counter = &ch->counter;
> > +	dev = &counter->dev;
> > +
> > +	/* Acquire unique ID */
> > +	err = ida_alloc(&counter_ida, GFP_KERNEL);
> > +	if (err < 0) {
> > +		goto err_ida_alloc;
> > +	}
> > +	dev->id = err;
> > +
> > +	err = counter_chrdev_add(counter);
> 
> Should think about renaming  counter_chrdev_add() given it
> does init and allocation stuff but no adding.

This is orthogonal to this series though. So I won't address this in the
context of the lifetime fixes here.

> Personal inclination here would be to keep the elements in here
> in the same order as in counter_register() where it doesn't matter
> in the interests of slightly easier comparison of the code.

I reordered a bit because counter_register fails to undo ida_alloc() in
the error path. However I might have missed that some initialisation has
to be done before calling counter_chrdev_add().

Will check in detail for v3.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)