| 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
| ||
|
Message-ID: <m4rcdq452qrlsdk7lbwt6pq2qlpbat6trvgufl7erope5zojxz@ilsgzm2lquwt>
Date: Thu, 13 Feb 2025 23:15:24 +0900
From: Koichiro Den <koichiro.den@...onical.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: linux-gpio@...r.kernel.org, brgl@...ev.pl, linus.walleij@...aro.org,
maciej.borzecki@...onical.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 08/10] gpio: aggregator: expoose aggregator created
via legacy sysfs to configfs
On Wed, Feb 12, 2025 at 04:36:13PM GMT, Geert Uytterhoeven wrote:
> Hi Den-san,
>
> Thanks for your patch!
>
> On Mon, 3 Feb 2025 at 04:14, Koichiro Den <koichiro.den@...onical.com> wrote:
> > Expose settings for aggregators created using the sysfs 'new_device'
> > interface to configfs. Once written to 'new_device', an "_auto.<N>" path
> > appears in the configfs regardless of whether the probe succeeds.
> > Consequently, users can no longer use that prefix for custom GPIO
> > aggregator names. The 'live' attribute changes to 1 when the probe
> > succeeds and the GPIO forwarder is instantiated.
> >
> > Note that the aggregator device created via sysfs is asynchrnous, i.e.
>
> asynchronous
>
> > writing into 'new_device' returns without waiting for probe completion,
> > and the probe may succeed, fail, or eventually succeed via deferred
> > probe. Thus, the 'live' attribute may change from 0 to 1 asynchronously
> > without notice. So, editting key/offset/name while it's waiting for
>
> editing
>
> > deferred probe is prohibited.
> >
> > The configfs auto-generation relies on create_default_group(), which
> > inherently prohibits rmdir(2). To align with the limitation, this commit
> > also prohibits mkdir(2) for them. When users want to change the number
> > of lines for an aggregator initialized via 'new_device', they need to
> > tear down the device using 'delete_device' and reconfigure it from
> > scratch. This does not break previous behaviour; users of legacy sysfs
> > interface simply gain additional almost read-only configfs exposure.
> >
> > Still, users can write into 'live' attribute to toggle the device unless
>
> write to the
>
> > it's waiting for deferred probe. So once probe succeeds, they can
> > deactivate it in the same manner as the devices initialized via
> > configfs.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@...onical.com>
>
> > @@ -73,6 +76,10 @@ struct gpio_aggregator_line {
> > enum gpio_lookup_flags flags;
> > };
> >
> > +struct gpio_aggregator_pdev_meta {
> > + bool init_via_sysfs;
> > +};
Thanks for pointing out all of them above. Will fix them all in v3.
>
> The use of this structure to indicate the instantiation method looks
> a bit hacky to me, but I don't see a better way...
I agree.
>
> > +
> > static DEFINE_MUTEX(gpio_aggregator_lock); /* protects idr */
> > static DEFINE_IDR(gpio_aggregator_idr);
> >
> > @@ -127,6 +134,14 @@ static bool aggr_is_active(struct gpio_aggregator *aggr)
> > return !!aggr->pdev && platform_get_drvdata(aggr->pdev);
> > }
> >
> > +/* Only aggregators created via legacy sysfs can be "activating". */
> > +static bool aggr_is_activating(struct gpio_aggregator *aggr)
> > +{
> > + lockdep_assert_held(&aggr->lock);
> > +
> > + return !!aggr->pdev && !platform_get_drvdata(aggr->pdev);
>
> No need for "!!".
Will fix in v3.
>
> > +}
> > +
> > static size_t aggr_count_lines(struct gpio_aggregator *aggr)
> > {
> > lockdep_assert_held(&aggr->lock);
>
> > @@ -1002,6 +1048,14 @@ gpio_aggr_make_group(struct config_group *group, const char *name)
> > if (!aggr)
> > return ERR_PTR(-ENOMEM);
> >
> > + /*
> > + * "_auto" prefix is reserved for auto-generated config group
> > + * for devices create via legacy sysfs interface.
> > + */
> > + if (strncmp(name, AGGREGATOR_LEGACY_PREFIX,
> > + sizeof(AGGREGATOR_LEGACY_PREFIX)) == 0)
> > + return ERR_PTR(-EINVAL);
>
> Missing kfree(aggr) in case of failure.
My bad. Will fix in v3.
>
> > +
> > mutex_lock(&gpio_aggregator_lock);
> > aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> > mutex_unlock(&gpio_aggregator_lock);
> > @@ -1044,6 +1098,8 @@ static struct configfs_subsystem gpio_aggr_subsys = {
> > static int aggr_parse(struct gpio_aggregator *aggr)
> > {
> > char *args = skip_spaces(aggr->args);
> > + struct gpio_aggregator_line *line;
> > + char name[CONFIGFS_ITEM_NAME_LEN];
> > char *key, *offsets, *p;
> > unsigned int i, n = 0;
> > int error = 0;
> > @@ -1055,14 +1111,29 @@ static int aggr_parse(struct gpio_aggregator *aggr)
> >
> > args = next_arg(args, &key, &p);
> > while (*args) {
> > + scnprintf(name, CONFIGFS_ITEM_NAME_LEN, "line%u", n);
>
> sizeof(name), to protect against future changes of name[].
Right, will fix it in v3.
>
> > static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> > size_t count)
> > {
> > + struct gpio_aggregator_pdev_meta meta;
>
> You might as well pre-initialize this:
>
> = { .init_via_sysfs = true };
>
Will do so in v3.
> > + char name[CONFIGFS_ITEM_NAME_LEN];
> > struct gpio_aggregator *aggr;
> > struct platform_device *pdev;
> > int res, id;
> > @@ -1112,6 +1210,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> >
> > memcpy(aggr->args, buf, count + 1);
> >
> > + aggr->init_via_sysfs = true;
> > aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
> > GFP_KERNEL);
> > if (!aggr->lookups) {
> > @@ -1128,10 +1227,22 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> > goto free_table;
> > }
> >
> > + scnprintf(name, CONFIGFS_ITEM_NAME_LEN,
>
> sizeof(name)
Will fix in v3.
>
> > + "%s.%d", AGGREGATOR_LEGACY_PREFIX, id);
> > + INIT_LIST_HEAD(&aggr->list_head);
> > + mutex_init(&aggr->lock);
> > + config_group_init_type_name(&aggr->group, name, &gpio_aggr_device_type);
> > + init_completion(&aggr->probe_completion);
>
> The code above is now almost identical to gpio_aggr_make_group().
(As I replied to your feedback to [PATCH v2 02/10]) I'll factor out the
common part and add new funcs, then use them in each.
>
> > +
> > + /* Expose to configfs */
> > + res = configfs_register_group(&gpio_aggr_subsys.su_group, &aggr->group);
> > + if (res)
> > + goto remove_idr;
> > +
> > aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
> > if (!aggr->lookups->dev_id) {
> > res = -ENOMEM;
> > - goto remove_idr;
> > + goto unregister_group;
> > }
> >
> > res = aggr_parse(aggr);
> > @@ -1140,7 +1251,9 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> >
> > gpiod_add_lookup_table(aggr->lookups);
> >
> > - pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
> > + meta.init_via_sysfs = true;
> > +
> > + pdev = platform_device_register_data(NULL, DRV_NAME, id, &meta, sizeof(meta));
> > if (IS_ERR(pdev)) {
> > res = PTR_ERR(pdev);
> > goto remove_table;
>
> > @@ -1258,7 +1379,26 @@ static struct platform_driver gpio_aggregator_driver = {
> >
> > static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
> > {
> > - aggr_free(p);
> > + /*
> > + * There should be no aggregator created via configfs, as their
> > + * presence would prevent module unloading.
> > + */
> > + struct gpio_aggregator *aggr = (struct gpio_aggregator *)p;
>
> The cast is not needed.
Will fix in v3.
Thanks!
Koichiro
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
Powered by blists - more mailing lists