Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1621475 > unrolled thread

[PATCH 0/4] serdev: fix broken lifetime assumptions

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] serdev: fix broken lifetime assumptions Johan Hovold <johan@kernel.org> - 2017-04-11 19:10 +0200
    [PATCH 2/4] serdev: fix tty-port client deregistration Johan Hovold <johan@kernel.org> - 2017-04-11 19:10 +0200
    [PATCH 1/4] Revert "tty_port: register tty ports with serdev bus" Johan Hovold <johan@kernel.org> - 2017-04-11 19:10 +0200
      Re: [PATCH 1/4] Revert "tty_port: register tty ports with serdev bus" Johan Hovold <johan@kernel.org> - 2017-04-12 16:40 +0200
    [PATCH 3/4] tty/serdev: add serdev registration interface Johan Hovold <johan@kernel.org> - 2017-04-11 19:10 +0200
      Re: [PATCH 3/4] tty/serdev: add serdev registration interface Rob Herring <robh@kernel.org> - 2017-04-20 20:00 +0200
        Re: [PATCH 3/4] tty/serdev: add serdev registration interface Johan Hovold <johan@kernel.org> - 2017-04-21 20:30 +0200

#1621475 — [PATCH 0/4] serdev: fix broken lifetime assumptions

This series fixes a number of issues with the new serdev code, which was
based on incorrect tty-port lifetime assumptions.

The first patch disables serdev support by reverting the patch which
hooked into the tty layer in a broken way that leads to crashes and
leaks when deregistering devices. This one should probably go into 4.11.

The second patch fixes a specific bug in the tty-port client
registration code, while the third patch adds a new interface for
registering serdev devices. The final patch ultimately enables serdev
again for the serial drivers.

More details can be found in the individual commit messages.

Johan


Johan Hovold (4):
  Revert "tty_port: register tty ports with serdev bus"
  serdev: fix tty-port client deregistration
  tty/serdev: add serdev registration interface
  serial: enable serdev support

 drivers/tty/serdev/serdev-ttyport.c | 21 +++++++----
 drivers/tty/serial/serial_core.c    |  4 +-
 drivers/tty/tty_port.c              | 74 ++++++++++++++++++++++++++++++++++---
 include/linux/serdev.h              |  7 +++-
 include/linux/tty.h                 |  9 +++++
 5 files changed, 99 insertions(+), 16 deletions(-)

-- 
2.12.2

[toc] | [next] | [standalone]

#1621476 — [PATCH 2/4] serdev: fix tty-port client deregistration

The port client data must be set when registering the serdev controller
or client deregistration will fail (and the serdev devices are left
registered and allocated) if the port was never opened in between.

Make sure to clear the port client data on any probe errors to avoid a
use-after-free when the client is later deregistered unconditionally
(e.g. in a tty-port deregistration helper).

Also move port client operation initialisation to registration. Note
that the client ops must be restored on failed probe.

Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index d05393594f15..06e17faa6dfb 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -101,9 +101,6 @@ static int ttyport_open(struct serdev_controller *ctrl)
 		return PTR_ERR(tty);
 	serport->tty = tty;
 
-	serport->port->client_ops = &client_ops;
-	serport->port->client_data = ctrl;
-
 	if (tty->ops->open)
 		tty->ops->open(serport->tty, NULL);
 	else
@@ -181,6 +178,7 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 					struct device *parent,
 					struct tty_driver *drv, int idx)
 {
+	const struct tty_port_client_operations *old_ops;
 	struct serdev_controller *ctrl;
 	struct serport *serport;
 	int ret;
@@ -199,15 +197,22 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 
 	ctrl->ops = &ctrl_ops;
 
+	old_ops = port->client_ops;
+	port->client_ops = &client_ops;
+	port->client_data = ctrl;
+
 	ret = serdev_controller_add(ctrl);
 	if (ret)
-		goto err_controller_put;
+		goto err_reset_data;
 
 	dev_info(&ctrl->dev, "tty port %s%d registered\n", drv->name, idx);
 	return &ctrl->dev;
 
-err_controller_put:
+err_reset_data:
+	port->client_data = NULL;
+	port->client_ops = old_ops;
 	serdev_controller_put(ctrl);
+
 	return ERR_PTR(ret);
 }
 
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1621478 — [PATCH 1/4] Revert "tty_port: register tty ports with serdev bus"

This reverts commit 8ee3fde047589dc9c201251f07d0ca1dc776feca.

The new serdev bus hooked into the tty layer in
tty_port_register_device() by registering a serdev controller instead of
a tty device whenever a serdev client is present, and by deregistering
the controller in the tty-port destructor. This is broken in several
ways:

Firstly, it leads to a NULL-pointer dereference whenever a tty driver
later deregisters its devices as no corresponding character device will
exist.

Secondly, far from every tty driver uses tty-port refcounting (e.g.
serial core) so the serdev devices might never be deregistered or
deallocated.

Thirdly, deregistering at tty-port destruction is too late as the
underlying device and structures may be long gone by then. A port is not
released before an open tty device is closed, something which a
registered serdev client can prevent from ever happening. A driver
callback while the device is gone typically also leads to crashes.

Many tty drivers even keep their ports around until the driver is
unloaded (e.g. serial core), something which even if a late callback
never happens, leads to leaks if a device is unbound from its driver and
is later rebound.

The right solution here is to add a new tty_port_unregister_device()
helper and to never call tty_device_unregister() whenever the port has
been claimed by serdev, but since this requires modifying just about
every tty driver (and multiple subsystems) it will need to be done
incrementally.

Reverting the offending patch is the first step in fixing the broken
lifetime assumptions. A follow-up patch will add a new pair of
tty-device registration helpers, which a vetted tty driver can use to
support serdev (initially serial core). When every tty driver uses the
serdev helpers (at least for deregistration), we can add serdev
registration to tty_port_register_device() again.

Note that this also fixes another issue with serdev, which currently
allocates and registers a serdev controller for every tty device
registered using tty_port_device_register() only to immediately
deregister and deallocate it when the corresponding OF node or serdev
child node is missing. This should be addressed before enabling serdev
for hot-pluggable buses.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/tty_port.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 1d21a9c1d33e..0c880f17d27e 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -16,7 +16,6 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/module.h>
-#include <linux/serdev.h>
 
 static int tty_port_default_receive_buf(struct tty_port *port,
 					const unsigned char *p,
@@ -129,15 +128,7 @@ struct device *tty_port_register_device_attr(struct tty_port *port,
 		struct device *device, void *drvdata,
 		const struct attribute_group **attr_grp)
 {
-	struct device *dev;
-
 	tty_port_link_device(port, driver, index);
-
-	dev = serdev_tty_port_register(port, device, driver, index);
-	if (PTR_ERR(dev) != -ENODEV)
-		/* Skip creating cdev if we registered a serdev device */
-		return dev;
-
 	return tty_register_device_attr(driver, index, device, drvdata,
 			attr_grp);
 }
@@ -189,9 +180,6 @@ static void tty_port_destructor(struct kref *kref)
 	/* check if last port ref was dropped before tty release */
 	if (WARN_ON(port->itty))
 		return;
-
-	serdev_tty_port_unregister(port);
-
 	if (port->xmit_buf)
 		free_page((unsigned long)port->xmit_buf);
 	tty_port_destroy(port);
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1622188 — Re: [PATCH 1/4] Revert "tty_port: register tty ports with serdev bus"

On Tue, Apr 11, 2017 at 08:53:32PM -0500, Rob Herring wrote:
> On Tue, Apr 11, 2017 at 12:07 PM, Johan Hovold <johan@kernel.org> wrote:
> > This reverts commit 8ee3fde047589dc9c201251f07d0ca1dc776feca.
> >
> > The new serdev bus hooked into the tty layer in
> > tty_port_register_device() by registering a serdev controller instead of
> > a tty device whenever a serdev client is present, and by deregistering
> > the controller in the tty-port destructor. This is broken in several
> > ways:
> 
> Just curious, how are you testing this? greybus?

By unbinding a platform device from its serial driver, and by using some
instrumentation code in USB serial.

> > Firstly, it leads to a NULL-pointer dereference whenever a tty driver
> > later deregisters its devices as no corresponding character device will
> > exist.
> >
> > Secondly, far from every tty driver uses tty-port refcounting (e.g.
> > serial core) so the serdev devices might never be deregistered or
> > deallocated.
> >
> > Thirdly, deregistering at tty-port destruction is too late as the
> > underlying device and structures may be long gone by then. A port is not
> > released before an open tty device is closed, something which a
> > registered serdev client can prevent from ever happening. A driver
> > callback while the device is gone typically also leads to crashes.
> >
> > Many tty drivers even keep their ports around until the driver is
> > unloaded (e.g. serial core), something which even if a late callback
> > never happens, leads to leaks if a device is unbound from its driver and
> > is later rebound.
> 
> Isn't this something we want to fix? i.e. everything done in
> probe/release rather than in init/exit.

It keeps some buffers allocated for a while longer than necessary, sure.
But there's quite a few drivers to change and for (non-hotpluggable)
serial drivers it doesn't make that much of difference as I assume
unbinding is mostly done for development purposes.

But we definitely need to make sure all drivers deregisters their
tty/serdev devices before starting to release resources.

> > The right solution here is to add a new tty_port_unregister_device()
> > helper and to never call tty_device_unregister() whenever the port has
> > been claimed by serdev, but since this requires modifying just about
> > every tty driver (and multiple subsystems) it will need to be done
> > incrementally.
> >
> > Reverting the offending patch is the first step in fixing the broken
> > lifetime assumptions. A follow-up patch will add a new pair of
> > tty-device registration helpers, which a vetted tty driver can use to
> > support serdev (initially serial core). When every tty driver uses the
> > serdev helpers (at least for deregistration), we can add serdev
> > registration to tty_port_register_device() again.
> 
> Reverting for 4.11 or not probably isn't that important. There aren't
> any users. I guess if you have a DT with a device added, then you
> could still have some problems.

True, but it only takes a DT child node (with a compatible string) to
trigger, and it's also the overhead added for all users of
tty_port_register_device() (e.g. cdc-acm) mentioned below. And we'd also
avoid enabling something in 4.11 which is then again disabled in 4.12
(for most tty drivers).

> > Note that this also fixes another issue with serdev, which currently
> > allocates and registers a serdev controller for every tty device
> > registered using tty_port_device_register() only to immediately
> > deregister and deallocate it when the corresponding OF node or serdev
> > child node is missing. This should be addressed before enabling serdev
> > for hot-pluggable buses.
> 
> Yeah, hot-plugging is definitely not supported ATM. Though folks are
> already asking about it. We need to figure out how to switch between a
> cdev and serdev.

Yeah, we need to give hotplugging some thought so we don't paint
ourselves into a corner here.

> Generally, the series looks good to me. Thanks for finding and fixing
> these issues. I'll give it a spin soon.

Thanks,
Johan

[toc] | [prev] | [next] | [standalone]

#1621482 — [PATCH 3/4] tty/serdev: add serdev registration interface

Add a new interface for registering a serdev controller and clients, and
a helper function to deregister serdev devices (or a tty device) that
were previously registered using the new interface.

Once every driver currently using the tty_port_register_device() helpers
have been vetted and converted to use the new serdev registration
interface (at least for deregistration), we can move serdev registration
to the current helpers and get rid of the serdev-specific functions.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c |  6 ++-
 drivers/tty/tty_port.c              | 76 +++++++++++++++++++++++++++++++++++++
 include/linux/serdev.h              |  7 +++-
 include/linux/tty.h                 |  9 +++++
 4 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index 06e17faa6dfb..8f9d7a0c27d1 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -216,16 +216,18 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 	return ERR_PTR(ret);
 }
 
-void serdev_tty_port_unregister(struct tty_port *port)
+int serdev_tty_port_unregister(struct tty_port *port)
 {
 	struct serdev_controller *ctrl = port->client_data;
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
 
 	if (!serport)
-		return;
+		return -ENODEV;
 
 	serdev_controller_remove(ctrl);
 	port->client_ops = NULL;
 	port->client_data = NULL;
 	serdev_controller_put(ctrl);
+
+	return 0;
 }
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 0c880f17d27e..2b4c62189346 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -16,6 +16,7 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/serdev.h>
 
 static int tty_port_default_receive_buf(struct tty_port *port,
 					const unsigned char *p,
@@ -134,6 +135,81 @@ struct device *tty_port_register_device_attr(struct tty_port *port,
 }
 EXPORT_SYMBOL_GPL(tty_port_register_device_attr);
 
+/**
+ * tty_port_register_device_attr_serdev - register tty or serdev device
+ * @port: tty_port of the device
+ * @driver: tty_driver for this device
+ * @index: index of the tty
+ * @device: parent if exists, otherwise NULL
+ * @drvdata: driver data for the device
+ * @attr_grp: attribute group for the device
+ *
+ * Register a serdev or tty device depending on if the parent device has any
+ * defined serdev clients or not.
+ */
+struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
+		struct tty_driver *driver, unsigned index,
+		struct device *device, void *drvdata,
+		const struct attribute_group **attr_grp)
+{
+	struct device *dev;
+
+	tty_port_link_device(port, driver, index);
+
+	dev = serdev_tty_port_register(port, device, driver, index);
+	if (PTR_ERR(dev) != -ENODEV) {
+		/* Skip creating cdev if we registered a serdev device */
+		return dev;
+	}
+
+	return tty_register_device_attr(driver, index, device, drvdata,
+			attr_grp);
+}
+EXPORT_SYMBOL_GPL(tty_port_register_device_attr_serdev);
+
+/**
+ * tty_port_register_device_serdev - register tty or serdev device
+ * @port: tty_port of the device
+ * @driver: tty_driver for this device
+ * @index: index of the tty
+ * @device: parent if exists, otherwise NULL
+ *
+ * Register a serdev or tty device depending on if the parent device has any
+ * defined serdev clients or not.
+ */
+struct device *tty_port_register_device_serdev(struct tty_port *port,
+		struct tty_driver *driver, unsigned index,
+		struct device *device)
+{
+	return tty_port_register_device_attr_serdev(port, driver, index,
+			device, NULL, NULL);
+}
+EXPORT_SYMBOL_GPL(tty_port_register_device_serdev);
+
+
+/**
+ * tty_port_unregister_device - deregister a tty or serdev device
+ * @port: tty_port of the device
+ * @driver: tty_driver for this device
+ * @index: index of the tty
+ *
+ * If a tty or serdev device is registered with a call to
+ * tty_port_register_device_serdev() then this function must be called when
+ * the device is gone.
+ */
+void tty_port_unregister_device(struct tty_port *port,
+		struct tty_driver *driver, unsigned index)
+{
+	int ret;
+
+	ret = serdev_tty_port_unregister(port);
+	if (ret == 0)
+		return;
+
+	tty_unregister_device(driver, index);
+}
+EXPORT_SYMBOL_GPL(tty_port_unregister_device);
+
 int tty_port_alloc_xmit_buf(struct tty_port *port)
 {
 	/* We may sleep in get_zeroed_page() */
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 9519da6253a8..d612953c63c2 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -248,7 +248,7 @@ struct tty_driver;
 struct device *serdev_tty_port_register(struct tty_port *port,
 					struct device *parent,
 					struct tty_driver *drv, int idx);
-void serdev_tty_port_unregister(struct tty_port *port);
+int serdev_tty_port_unregister(struct tty_port *port);
 #else
 static inline struct device *serdev_tty_port_register(struct tty_port *port,
 					   struct device *parent,
@@ -256,7 +256,10 @@ static inline struct device *serdev_tty_port_register(struct tty_port *port,
 {
 	return ERR_PTR(-ENODEV);
 }
-static inline void serdev_tty_port_unregister(struct tty_port *port) {}
+static inline int serdev_tty_port_unregister(struct tty_port *port)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_SERIAL_DEV_CTRL_TTYPORT */
 
 #endif /*_LINUX_SERDEV_H */
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1017e904c0a3..0a3eeca4761a 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -555,6 +555,15 @@ extern struct device *tty_port_register_device_attr(struct tty_port *port,
 		struct tty_driver *driver, unsigned index,
 		struct device *device, void *drvdata,
 		const struct attribute_group **attr_grp);
+extern struct device *tty_port_register_device_serdev(struct tty_port *port,
+		struct tty_driver *driver, unsigned index,
+		struct device *device);
+extern struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
+		struct tty_driver *driver, unsigned index,
+		struct device *device, void *drvdata,
+		const struct attribute_group **attr_grp);
+extern void tty_port_unregister_device(struct tty_port *port,
+		struct tty_driver *driver, unsigned index);
 extern int tty_port_alloc_xmit_buf(struct tty_port *port);
 extern void tty_port_free_xmit_buf(struct tty_port *port);
 extern void tty_port_destroy(struct tty_port *port);
-- 
2.12.2

[toc] | [prev] | [next] | [standalone]

#1627717 — Re: [PATCH 3/4] tty/serdev: add serdev registration interface

On Tue, Apr 11, 2017 at 12:07 PM, Johan Hovold <johan@kernel.org> wrote:
> Add a new interface for registering a serdev controller and clients, and
> a helper function to deregister serdev devices (or a tty device) that
> were previously registered using the new interface.
>
> Once every driver currently using the tty_port_register_device() helpers
> have been vetted and converted to use the new serdev registration
> interface (at least for deregistration), we can move serdev registration
> to the current helpers and get rid of the serdev-specific functions.

I don't really think this is necessary. While in theory any tty port
can work with serdev, the reality is it only ever going to be used
with a few. There's about 31 possible drivers. Of those, there's a
fair number that an attached device is not going to make sense (e.g.
ISDN CAPI, rfcomm?, goldfish, etc.). Second, right now serdev only
works with DT binding. There are only 3 drivers supporting DT:
serial_core, goldfish, and ehv_bytechan. Likely drivers I'm aware of
to use serdev in addition to serial_core are USB serial and greybus.
But for those, we currently only could support them if the whole bus
topology is described in DT. Otherwise, we first have to figure out
how to apply DT overlays to arbitrary devices not described in DT.

OTOH, we are at least explicit with what tty ports support serdev.

Rob

[toc] | [prev] | [next] | [standalone]

#1628440 — Re: [PATCH 3/4] tty/serdev: add serdev registration interface

On Thu, Apr 20, 2017 at 12:52:50PM -0500, Rob Herring wrote:
> On Tue, Apr 11, 2017 at 12:07 PM, Johan Hovold <johan@kernel.org> wrote:
> > Add a new interface for registering a serdev controller and clients, and
> > a helper function to deregister serdev devices (or a tty device) that
> > were previously registered using the new interface.
> >
> > Once every driver currently using the tty_port_register_device() helpers
> > have been vetted and converted to use the new serdev registration
> > interface (at least for deregistration), we can move serdev registration
> > to the current helpers and get rid of the serdev-specific functions.
> 
> I don't really think this is necessary. While in theory any tty port
> can work with serdev, the reality is it only ever going to be used
> with a few. There's about 31 possible drivers. Of those, there's a
> fair number that an attached device is not going to make sense (e.g.
> ISDN CAPI, rfcomm?, goldfish, etc.). Second, right now serdev only
> works with DT binding. There are only 3 drivers supporting DT:
> serial_core, goldfish, and ehv_bytechan.

There are also about ten PCI-based drivers (e.g. rocket.c), which, if
I'm not mistaken, could have an associated DT-node already today. And so
could the SPI-based ifx6x60 driver (modem).

> Likely drivers I'm aware of to use serdev in addition to serial_core
> are USB serial and greybus.  But for those, we currently only could
> support them if the whole bus topology is described in DT. Otherwise,
> we first have to figure out how to apply DT overlays to arbitrary
> devices not described in DT.

There's also cdc-acm and possibly fw-serial (in staging).

And while our current USB device-tree implementation is limited to
describing USB devices, extending this to interfaces, which our USB
drivers bind to, should be easily done (I'm looking into it now).

There's also a comment in serdev about adding support for "ACPI and
platform" descriptions, and you mentioned being able to switch between
cdev and serdev as a possible future extension.

The point is that serdev currently hooks into the tty layer through a
generic function, and if and how a client can be described is just
an implementation detail. Rather than using such a large hammer, it
seems to me that enabling serdev on a per-driver basis after making sure
that nothing breaks (e.g. resources are released on deregistration) is
preferred.

> OTOH, we are at least explicit with what tty ports support serdev.

That would be another benefit.

Johan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web