linux-kernel - Re: [RFC PATCH 7/7] i2c: core: hand over reserved devices when requesting ancillary addresses

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: <20200313124214.GA1299@ninjato>
Date:   Fri, 13 Mar 2020 13:42:14 +0100
From:   Wolfram Sang <wsa@...-dreams.de>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Linux I2C <linux-i2c@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        linux-i3c@...ts.infradead.org,
        Kieran Bingham <kieran@...uared.org.uk>,
        Niklas Söderlund <niklas.soderlund@...natech.se>,
        Luca Ceresoli <luca@...aceresoli.net>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Vladimir Zapolskiy <vz@...ia.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>
Subject: Re: [RFC PATCH 7/7] i2c: core: hand over reserved devices when
 requesting ancillary addresses


> > @@ -984,7 +986,21 @@ struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
> >                         of_property_read_u32_index(np, "reg", i, &addr);
> >         }
> >
> > -       dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
> > +       dev_info(adapter_dev, "Address for %s : 0x%x\n", name, addr);
> > +
> > +       /* No need to scan muxes, siblings must sit on the same adapter */
> > +       reserved_dev = device_find_child(adapter_dev, &addr, __i2c_check_addr_busy);
> > +       reserved_client = i2c_verify_client(reserved_dev);
> > +
> > +       if (reserved_client) {
> > +               if (reserved_client->dev.of_node != np ||
> > +                   strcmp(reserved_client->name, I2C_RESERVED_DRV_NAME) != 0)
> > +                       return ERR_PTR(-EBUSY);
> 
> Missing put_device(reserved_dev).

Actually, I think the code could even be like this:

	struct i2c_client *reserved_client = NULL;

	...

	reserved_dev = device_find_child(adapter_dev, &addr, __i2c_check_addr_busy);
	if (reserved_dev) {
		reserved_np = reserved_dev->of_node;
		reserved_client = i2c_verify_client(reserved_dev);
		put_device(reserved_dev);
	}

	if (reserved_client) {
		if (reserved_np != np ||
		    strcmp(reserved_client->name, I2C_RESERVED_DRV_NAME) != 0)
			return ERR_PTR(-EBUSY);

		strlcpy(reserved_client->name, I2C_DUMMY_DRV_NAME, sizeof(client->name));
		return reserved_client;
	}

	return i2c_new_dummy_device(client->adapter, addr);

We put the device early - as soon we don't access the struct anymore. I
think we don't need the refcnt any further because what we are doing
here is to hand over the initial refcnt from the core to the requesting
driver. We turn the device from "reserved" (internally managed) to
"dummy" (managed by the driver). So, I think the code is okay regarding
the struct device. I will have a second look when it comes to
concurrency problems regarding the struct i2c_client, though.

> (perhaps i2c_verify_client() checking dev was not such a great idea, as
>  callers need to act on dev && !verified anyway?)

Yeah, since I refactored the ACPI code as well, patch 1 from this series
can probably go.

Thanks again for your review, Geert!


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