staging: greybus: uart: fix tty use after free

PATCH In review Johan Hovold 2 participants 4 messages Sep 6, 2021 – Sep 8, 2021

Subsystems (4)

Stable branch Staging Greybus Greybus UART protocols drivers

lore

Share & tools

lore

Johan Hovold

author maintainer

2021-09-06 12:48 UTC #0 lore raw

PATCH 1/1 staging: greybus: uart: fix tty use after free

User space can hold a tty open indefinitely and tty drivers must not release the underlying structures until the last user is gone.

Switch to using the tty-port reference counter to manage the life time of the greybus tty state to avoid use after free after a disconnect.

Fixes a18e15175708 ("greybus: more uart work")

Cc [email protected] # 4.9

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/staging/greybus/uart.c | 62 ++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 30 deletions(-)

drivers/staging/greybus/uart.c +32 ~~-30~~

index 73f01ed1e5b7..a943fce322be 100644
@@ -761,6 +761,17 @@ static void gb_tty_port_shutdown(struct tty_port *port)
 	gbphy_runtime_put_autosuspend(gb_tty->gbphy_dev);
 }
 
+static void gb_tty_port_destruct(struct tty_port *port)
+{
+	struct gb_tty *gb_tty = container_of(port, struct gb_tty, port);
+
+	if (gb_tty->minor != GB_NUM_MINORS)
+		release_minor(gb_tty);
+	kfifo_free(&gb_tty->write_fifo);
+	kfree(gb_tty->buffer);
+	kfree(gb_tty);
+}
+
 static const struct tty_operations gb_ops = {
 	.install =		gb_tty_install,
 	.open =			gb_tty_open,

@@ -786,6 +797,7 @@ static const struct tty_port_operations gb_port_ops = {
 	.dtr_rts =		gb_tty_dtr_rts,
 	.activate =		gb_tty_port_activate,
 	.shutdown =		gb_tty_port_shutdown,
+	.destruct =		gb_tty_port_destruct,
 };
 
 static int gb_uart_probe(struct gbphy_device *gbphy_dev,

@@ -798,17 +810,11 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 	int retval;
 	int minor;
 
-	gb_tty = kzalloc(sizeof(*gb_tty), GFP_KERNEL);
-	if (!gb_tty)
-		return -ENOMEM;
-
 	connection = gb_connection_create(gbphy_dev->bundle,
 					  le16_to_cpu(gbphy_dev->cport_desc->id),
 					  gb_uart_request_handler);
-	if (IS_ERR(connection)) {
-		retval = PTR_ERR(connection);
-		goto exit_tty_free;
-	}
+	if (IS_ERR(connection))
+		return PTR_ERR(connection);
 
 	max_payload = gb_operation_get_payload_size_max(connection);
 	if (max_payload < sizeof(struct gb_uart_send_data_request)) {

@@ -816,13 +822,23 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 		goto exit_connection_destroy;
 	}
 
+	gb_tty = kzalloc(sizeof(*gb_tty), GFP_KERNEL);
+	if (!gb_tty) {
+		retval = -ENOMEM;
+		goto exit_connection_destroy;
+	}
+
+	tty_port_init(&gb_tty->port);
+	gb_tty->port.ops = &gb_port_ops;
+	gb_tty->minor = GB_NUM_MINORS;
+
 	gb_tty->buffer_payload_max = max_payload -
 			sizeof(struct gb_uart_send_data_request);
 
 	gb_tty->buffer = kzalloc(gb_tty->buffer_payload_max, GFP_KERNEL);
 	if (!gb_tty->buffer) {
 		retval = -ENOMEM;
-		goto exit_connection_destroy;
+		goto exit_put_port;
 	}
 
 	INIT_WORK(&gb_tty->tx_work, gb_uart_tx_write_work);

@@ -830,7 +846,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 	retval = kfifo_alloc(&gb_tty->write_fifo, GB_UART_WRITE_FIFO_SIZE,
 			     GFP_KERNEL);
 	if (retval)
-		goto exit_buf_free;
+		goto exit_put_port;
 
 	gb_tty->credits = GB_UART_FIRMWARE_CREDITS;
 	init_completion(&gb_tty->credits_complete);

@@ -844,7 +860,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 		} else {
 			retval = minor;
 		}
-		goto exit_kfifo_free;
+		goto exit_put_port;
 	}
 
 	gb_tty->minor = minor;

@@ -853,9 +869,6 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 	init_waitqueue_head(&gb_tty->wioctl);
 	mutex_init(&gb_tty->mutex);
 
-	tty_port_init(&gb_tty->port);
-	gb_tty->port.ops = &gb_port_ops;
-
 	gb_tty->connection = connection;
 	gb_tty->gbphy_dev = gbphy_dev;
 	gb_connection_set_data(connection, gb_tty);

@@ -863,7 +876,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 
 	retval = gb_connection_enable_tx(connection);
 	if (retval)
-		goto exit_release_minor;
+		goto exit_put_port;
 
 	send_control(gb_tty, gb_tty->ctrlout);

@@ -890,16 +903,10 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 
 exit_connection_disable:
 	gb_connection_disable(connection);
-exit_release_minor:
-	release_minor(gb_tty);
-exit_kfifo_free:
-	kfifo_free(&gb_tty->write_fifo);
-exit_buf_free:
-	kfree(gb_tty->buffer);
+exit_put_port:
+	tty_port_put(&gb_tty->port);
 exit_connection_destroy:
 	gb_connection_destroy(connection);
-exit_tty_free:
-	kfree(gb_tty);
 
 	return retval;
 }

@@ -930,15 +937,10 @@ static void gb_uart_remove(struct gbphy_device *gbphy_dev)
 	gb_connection_disable_rx(connection);
 	tty_unregister_device(gb_tty_driver, gb_tty->minor);
 
-	/* FIXME - free transmit / receive buffers */
-
 	gb_connection_disable(connection);
-	tty_port_destroy(&gb_tty->port);
 	gb_connection_destroy(connection);
-	release_minor(gb_tty);
-	kfifo_free(&gb_tty->write_fifo);
-	kfree(gb_tty->buffer);
-	kfree(gb_tty);
+
+	tty_port_put(&gb_tty->port);
 }
 
 static int gb_tty_init(void)

Signature · 1 line

2.32.0

Alex Elder

2021-09-07 13:32 UTC #1 lore raw

Quoted 9 lines · Johan Hovold

User space can hold a tty open indefinitely and tty drivers must not release the underlying structures until the last user is gone.
Switch to using the tty-port reference counter to manage the life time of the greybus tty state to avoid use after free after a disconnect.
Fixes a18e15175708 ("greybus: more uart work")
Cc [email protected] # 4.9
Signed-off-by Johan Hovold <[email protected]>

I have a couple of minor comments. I assume the
tty model matches normal patterns for get/put and
reference counted objects, and based on that I'm
ready to give a Reviewed-by, but I'd like to hear
your responses first.

-Alex

Quoted 9 lines

---
  drivers/staging/greybus/uart.c | 62 ++++++++++++++++++----------------
  1 file changed, 32 insertions(+), 30 deletions(-)

drivers/staging/greybus/uart.c +0 -0

index 73f01ed1e5b7..a943fce322be 100644

drivers/staging/greybus/uart.c static void gb_tty_port_shutdown(struct tty_port *port) · line 767

Quoted 7 lines

drivers/staging/greybus/uart.c +4 -0

@@ -761,6 +761,17 @@ static void gb_tty_port_shutdown(struct tty_port *port)
  	gbphy_runtime_put_autosuspend(gb_tty->gbphy_dev);
  }
 
+static void gb_tty_port_destruct(struct tty_port *port)
+{
+	struct gb_tty *gb_tty = container_of(port, struct gb_tty, port);
+

So the minor number is GB_NUM_MINORS until after both the buffer and the kfifo have been allocated. And kfifo_free() (similar to kfree()) handles being provided a non-initialized kfifo, correct?

Quoted 19 lines

drivers/staging/greybus/uart.c +8 -0

+	if (gb_tty->minor != GB_NUM_MINORS)
+		release_minor(gb_tty);
+	kfifo_free(&gb_tty->write_fifo);
+	kfree(gb_tty->buffer);
+	kfree(gb_tty);
+}
+
  static const struct tty_operations gb_ops = {
  	.install =		gb_tty_install,
  	.open =			gb_tty_open,

@@ -786,6 +797,7 @@ static const struct tty_port_operations gb_port_ops = {
  	.dtr_rts =		gb_tty_dtr_rts,
  	.activate =		gb_tty_port_activate,
  	.shutdown =		gb_tty_port_shutdown,
+	.destruct =		gb_tty_port_destruct,
  };
 
  static int gb_uart_probe(struct gbphy_device *gbphy_dev,

drivers/staging/greybus/uart.c static int gb_uart_probe(struct gbphy_device *gbphy_dev, · line 804

Quoted 7 lines

drivers/staging/greybus/uart.c +0 -4

@@ -798,17 +810,11 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
  	int retval;
  	int minor;
 
-	gb_tty = kzalloc(sizeof(*gb_tty), GFP_KERNEL);
-	if (!gb_tty)
-		return -ENOMEM;
-

Why do you reorder when you allocate the gb_tty structure? I don't have a problem with it, but it seems like the order doesn't matter. Is it just so you can initialize it right after it's allocated? (If so, I like that reason.)

Quoted 98 lines

drivers/staging/greybus/uart.c +18 ~~-20~~

  	connection = gb_connection_create(gbphy_dev->bundle,
  					  le16_to_cpu(gbphy_dev->cport_desc->id),
  					  gb_uart_request_handler);
-	if (IS_ERR(connection)) {
-		retval = PTR_ERR(connection);
-		goto exit_tty_free;
-	}
+	if (IS_ERR(connection))
+		return PTR_ERR(connection);
 
  	max_payload = gb_operation_get_payload_size_max(connection);
  	if (max_payload < sizeof(struct gb_uart_send_data_request)) {

@@ -816,13 +822,23 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
  		goto exit_connection_destroy;
  	}
 
+	gb_tty = kzalloc(sizeof(*gb_tty), GFP_KERNEL);
+	if (!gb_tty) {
+		retval = -ENOMEM;
+		goto exit_connection_destroy;
+	}
+
+	tty_port_init(&gb_tty->port);
+	gb_tty->port.ops = &gb_port_ops;
+	gb_tty->minor = GB_NUM_MINORS;
+
  	gb_tty->buffer_payload_max = max_payload -
  			sizeof(struct gb_uart_send_data_request);
 
  	gb_tty->buffer = kzalloc(gb_tty->buffer_payload_max, GFP_KERNEL);
  	if (!gb_tty->buffer) {
  		retval = -ENOMEM;
-		goto exit_connection_destroy;
+		goto exit_put_port;
  	}
 
  	INIT_WORK(&gb_tty->tx_work, gb_uart_tx_write_work);

@@ -830,7 +846,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
  	retval = kfifo_alloc(&gb_tty->write_fifo, GB_UART_WRITE_FIFO_SIZE,
  			     GFP_KERNEL);
  	if (retval)
-		goto exit_buf_free;
+		goto exit_put_port;
 
  	gb_tty->credits = GB_UART_FIRMWARE_CREDITS;
  	init_completion(&gb_tty->credits_complete);

@@ -844,7 +860,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
  		} else {
  			retval = minor;
  		}
-		goto exit_kfifo_free;
+		goto exit_put_port;
  	}
 
  	gb_tty->minor = minor;

@@ -853,9 +869,6 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
  	init_waitqueue_head(&gb_tty->wioctl);
  	mutex_init(&gb_tty->mutex);
 
-	tty_port_init(&gb_tty->port);
-	gb_tty->port.ops = &gb_port_ops;
-
  	gb_tty->connection = connection;
  	gb_tty->gbphy_dev = gbphy_dev;
  	gb_connection_set_data(connection, gb_tty);

@@ -863,7 +876,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 
  	retval = gb_connection_enable_tx(connection);
  	if (retval)
-		goto exit_release_minor;
+		goto exit_put_port;
 
  	send_control(gb_tty, gb_tty->ctrlout);

@@ -890,16 +903,10 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 
  exit_connection_disable:
  	gb_connection_disable(connection);
-exit_release_minor:
-	release_minor(gb_tty);
-exit_kfifo_free:
-	kfifo_free(&gb_tty->write_fifo);
-exit_buf_free:
-	kfree(gb_tty->buffer);
+exit_put_port:
+	tty_port_put(&gb_tty->port);
  exit_connection_destroy:
  	gb_connection_destroy(connection);
-exit_tty_free:
-	kfree(gb_tty);
 
  	return retval;
  }

@@ -930,15 +937,10 @@ static void gb_uart_remove(struct gbphy_device *gbphy_dev)
  	gb_connection_disable_rx(connection);
  	tty_unregister_device(gb_tty_driver, gb_tty->minor);
 
-	/* FIXME - free transmit / receive buffers */

drivers/staging/greybus/uart.c static void gb_uart_remove(struct gbphy_device *gbphy_dev) · line 943

Quoted 10 lines

drivers/staging/greybus/uart.c +2 -6

-
  	gb_connection_disable(connection);
-	tty_port_destroy(&gb_tty->port);
  	gb_connection_destroy(connection);
-	release_minor(gb_tty);
-	kfifo_free(&gb_tty->write_fifo);
-	kfree(gb_tty->buffer);
-	kfree(gb_tty);
+
+	tty_port_put(&gb_tty->port);

In the error path above, you call tty_port_put() before calling gb_connection_destroy(), which matches (in reverse) the order in which they're created. I'm accustomed to having the order of the calls here match the error path. Is this difference intentional? (It shouldn't really matter.)

-Alex

Quoted 4 lines

drivers/staging/greybus/uart.c +0 -0
  }
 
  static int gb_tty_init(void)

Johan Hovold

author maintainer

2021-09-08 07:57 UTC #2 lore raw

Quoted 9 lines · Alex Elder

drivers/staging/greybus/uart.c static void gb_tty_port_shutdown(struct tty_port *port) · line 767
Quoted 4 lines · Johan Hovold
drivers/staging/greybus/uart.c +4 -0
+static void gb_tty_port_destruct(struct tty_port *port)
+{
+	struct gb_tty *gb_tty = container_of(port, struct gb_tty, port);
+
So the minor number is GB_NUM_MINORS until after both the buffer and the kfifo have been allocated.

Yes, but more importantly until the minor number has been allocated.

Quoted 2 lines

And kfifo_free() (similar to
kfree()) handles being provided a non-initialized kfifo, correct?

Correct, as long as it has been zeroed.

Quoted 22 lines

drivers/staging/greybus/uart.c +8 -0

+	if (gb_tty->minor != GB_NUM_MINORS)
+		release_minor(gb_tty);
+	kfifo_free(&gb_tty->write_fifo);
+	kfree(gb_tty->buffer);
+	kfree(gb_tty);
+}
+
  static const struct tty_operations gb_ops = {
  	.install =		gb_tty_install,
  	.open =			gb_tty_open,

@@ -786,6 +797,7 @@ static const struct tty_port_operations gb_port_ops = {
  	.dtr_rts =		gb_tty_dtr_rts,
  	.activate =		gb_tty_port_activate,
  	.shutdown =		gb_tty_port_shutdown,
+	.destruct =		gb_tty_port_destruct,
  };
 
  static int gb_uart_probe(struct gbphy_device *gbphy_dev,

@@ -798,17 +810,11 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
  	int retval;
  	int minor;

Quoted 9 lines

drivers/staging/greybus/uart.c static int gb_uart_probe(struct gbphy_device *gbphy_dev, · line 804
Quoted 4 lines
drivers/staging/greybus/uart.c +0 -4
-	gb_tty = kzalloc(sizeof(*gb_tty), GFP_KERNEL);
-	if (!gb_tty)
-		return -ENOMEM;
-
Why do you reorder when you allocate the gb_tty structure? I don't have a problem with it, but it seems like the order doesn't matter. Is it just so you can initialize it right after it's allocated? (If so, I like that reason.)

Yeah, and I wanted to keep the bits managed by the port reference count together.

Quoted 107 lines

drivers/staging/greybus/uart.c +19 ~~-26~~

  	connection = gb_connection_create(gbphy_dev->bundle,
  					  le16_to_cpu(gbphy_dev->cport_desc->id),
  					  gb_uart_request_handler);
-	if (IS_ERR(connection)) {
-		retval = PTR_ERR(connection);
-		goto exit_tty_free;
-	}
+	if (IS_ERR(connection))
+		return PTR_ERR(connection);
 
  	max_payload = gb_operation_get_payload_size_max(connection);
  	if (max_payload < sizeof(struct gb_uart_send_data_request)) {

@@ -816,13 +822,23 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
  		goto exit_connection_destroy;
  	}
 
+	gb_tty = kzalloc(sizeof(*gb_tty), GFP_KERNEL);
+	if (!gb_tty) {
+		retval = -ENOMEM;
+		goto exit_connection_destroy;
+	}
+
+	tty_port_init(&gb_tty->port);
+	gb_tty->port.ops = &gb_port_ops;
+	gb_tty->minor = GB_NUM_MINORS;
+
  	gb_tty->buffer_payload_max = max_payload -
  			sizeof(struct gb_uart_send_data_request);
 
  	gb_tty->buffer = kzalloc(gb_tty->buffer_payload_max, GFP_KERNEL);
  	if (!gb_tty->buffer) {
  		retval = -ENOMEM;
-		goto exit_connection_destroy;
+		goto exit_put_port;
  	}
 
  	INIT_WORK(&gb_tty->tx_work, gb_uart_tx_write_work);

@@ -830,7 +846,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
  	retval = kfifo_alloc(&gb_tty->write_fifo, GB_UART_WRITE_FIFO_SIZE,
  			     GFP_KERNEL);
  	if (retval)
-		goto exit_buf_free;
+		goto exit_put_port;
 
  	gb_tty->credits = GB_UART_FIRMWARE_CREDITS;
  	init_completion(&gb_tty->credits_complete);

@@ -844,7 +860,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
  		} else {
  			retval = minor;
  		}
-		goto exit_kfifo_free;
+		goto exit_put_port;
  	}
 
  	gb_tty->minor = minor;

@@ -853,9 +869,6 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
  	init_waitqueue_head(&gb_tty->wioctl);
  	mutex_init(&gb_tty->mutex);
 
-	tty_port_init(&gb_tty->port);
-	gb_tty->port.ops = &gb_port_ops;
-
  	gb_tty->connection = connection;
  	gb_tty->gbphy_dev = gbphy_dev;
  	gb_connection_set_data(connection, gb_tty);

@@ -863,7 +876,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 
  	retval = gb_connection_enable_tx(connection);
  	if (retval)
-		goto exit_release_minor;
+		goto exit_put_port;
 
  	send_control(gb_tty, gb_tty->ctrlout);

@@ -890,16 +903,10 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 
  exit_connection_disable:
  	gb_connection_disable(connection);
-exit_release_minor:
-	release_minor(gb_tty);
-exit_kfifo_free:
-	kfifo_free(&gb_tty->write_fifo);
-exit_buf_free:
-	kfree(gb_tty->buffer);
+exit_put_port:
+	tty_port_put(&gb_tty->port);
  exit_connection_destroy:
  	gb_connection_destroy(connection);
-exit_tty_free:
-	kfree(gb_tty);
 
  	return retval;
  }

@@ -930,15 +937,10 @@ static void gb_uart_remove(struct gbphy_device *gbphy_dev)
  	gb_connection_disable_rx(connection);
  	tty_unregister_device(gb_tty_driver, gb_tty->minor);
 
-	/* FIXME - free transmit / receive buffers */
-
  	gb_connection_disable(connection);
-	tty_port_destroy(&gb_tty->port);
  	gb_connection_destroy(connection);
-	release_minor(gb_tty);
-	kfifo_free(&gb_tty->write_fifo);
-	kfree(gb_tty->buffer);
-	kfree(gb_tty);
+

Quoted 8 lines

Review static void gb_uart_remove(struct gbphy_device *gbphy_dev) · line 943
Quoted 1 line
(quoted diff) +1 -0
+	tty_port_put(&gb_tty->port);
In the error path above, you call tty_port_put() before calling gb_connection_destroy(), which matches (in reverse) the order in which they're created. I'm accustomed to having the order of the calls here match the error path. Is this difference intentional? (It shouldn't really matter.)

I considered reordering (it stands out a bit in my eyes too) but decided to leave it in place to highlight the fact that the connection may be freed before the rest of the state either way (since a user may hold a reference to it).

[ A driver mustn't do I/O after remove() returns even if it might be
possible to keep the connection structure around. That would however be complicated by the lack of reference counting on the bundle/interface structures so I decided not to venture down that path. ]

Like you say, the order here doesn't really matter so I can move it up if you prefer.

Johan

Alex Elder

Reviewed-by

2021-09-08 12:16 UTC #3 lore raw

Quoted 11 lines · Johan Hovold

Quoted 9 lines · Alex Elder
drivers/staging/greybus/uart.c static void gb_tty_port_shutdown(struct tty_port *port) · line 767
Quoted 4 lines · Johan Hovold
drivers/staging/greybus/uart.c +4 -0
+static void gb_tty_port_destruct(struct tty_port *port)
+{
+	struct gb_tty *gb_tty = container_of(port, struct gb_tty, port);
+
So the minor number is GB_NUM_MINORS until after both the buffer and the kfifo have been allocated.

Quoted 6 lines

Yes, but more importantly until the minor number has been allocated.
Quoted 2 lines
And kfifo_free() (similar to
kfree()) handles being provided a non-initialized kfifo, correct?
Correct, as long as it has been zeroed.

The original patch looks fine to me. Thanks for the explanation.

Reviewed-by Alex Elder <[email protected]>

Quoted 166 lines

Quoted 22 lines

drivers/staging/greybus/uart.c +8 -0

+	if (gb_tty->minor != GB_NUM_MINORS)
+		release_minor(gb_tty);
+	kfifo_free(&gb_tty->write_fifo);
+	kfree(gb_tty->buffer);
+	kfree(gb_tty);
+}
+
   static const struct tty_operations gb_ops = {
   	.install =		gb_tty_install,
   	.open =			gb_tty_open,

@@ -786,6 +797,7 @@ static const struct tty_port_operations gb_port_ops = {
   	.dtr_rts =		gb_tty_dtr_rts,
   	.activate =		gb_tty_port_activate,
   	.shutdown =		gb_tty_port_shutdown,
+	.destruct =		gb_tty_port_destruct,
   };
 
   static int gb_uart_probe(struct gbphy_device *gbphy_dev,

@@ -798,17 +810,11 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
   	int retval;
   	int minor;

Quoted 9 lines

drivers/staging/greybus/uart.c static int gb_uart_probe(struct gbphy_device *gbphy_dev, · line 804
Quoted 4 lines
drivers/staging/greybus/uart.c +0 -4
-	gb_tty = kzalloc(sizeof(*gb_tty), GFP_KERNEL);
-	if (!gb_tty)
-		return -ENOMEM;
-
Why do you reorder when you allocate the gb_tty structure? I don't have a problem with it, but it seems like the order doesn't matter. Is it just so you can initialize it right after it's allocated? (If so, I like that reason.)

Yeah, and I wanted to keep the bits managed by the port reference count together.

Quoted 107 lines

drivers/staging/greybus/uart.c +19 ~~-26~~

   	connection = gb_connection_create(gbphy_dev->bundle,
   					  le16_to_cpu(gbphy_dev->cport_desc->id),
   					  gb_uart_request_handler);
-	if (IS_ERR(connection)) {
-		retval = PTR_ERR(connection);
-		goto exit_tty_free;
-	}
+	if (IS_ERR(connection))
+		return PTR_ERR(connection);
 
   	max_payload = gb_operation_get_payload_size_max(connection);
   	if (max_payload < sizeof(struct gb_uart_send_data_request)) {

@@ -816,13 +822,23 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
   		goto exit_connection_destroy;
   	}
 
+	gb_tty = kzalloc(sizeof(*gb_tty), GFP_KERNEL);
+	if (!gb_tty) {
+		retval = -ENOMEM;
+		goto exit_connection_destroy;
+	}
+
+	tty_port_init(&gb_tty->port);
+	gb_tty->port.ops = &gb_port_ops;
+	gb_tty->minor = GB_NUM_MINORS;
+
   	gb_tty->buffer_payload_max = max_payload -
   			sizeof(struct gb_uart_send_data_request);
 
   	gb_tty->buffer = kzalloc(gb_tty->buffer_payload_max, GFP_KERNEL);
   	if (!gb_tty->buffer) {
   		retval = -ENOMEM;
-		goto exit_connection_destroy;
+		goto exit_put_port;
   	}
 
   	INIT_WORK(&gb_tty->tx_work, gb_uart_tx_write_work);

@@ -830,7 +846,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
   	retval = kfifo_alloc(&gb_tty->write_fifo, GB_UART_WRITE_FIFO_SIZE,
   			     GFP_KERNEL);
   	if (retval)
-		goto exit_buf_free;
+		goto exit_put_port;
 
   	gb_tty->credits = GB_UART_FIRMWARE_CREDITS;
   	init_completion(&gb_tty->credits_complete);

@@ -844,7 +860,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
   		} else {
   			retval = minor;
   		}
-		goto exit_kfifo_free;
+		goto exit_put_port;
   	}
 
   	gb_tty->minor = minor;

@@ -853,9 +869,6 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
   	init_waitqueue_head(&gb_tty->wioctl);
   	mutex_init(&gb_tty->mutex);
 
-	tty_port_init(&gb_tty->port);
-	gb_tty->port.ops = &gb_port_ops;
-
   	gb_tty->connection = connection;
   	gb_tty->gbphy_dev = gbphy_dev;
   	gb_connection_set_data(connection, gb_tty);

@@ -863,7 +876,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 
   	retval = gb_connection_enable_tx(connection);
   	if (retval)
-		goto exit_release_minor;
+		goto exit_put_port;
 
   	send_control(gb_tty, gb_tty->ctrlout);

@@ -890,16 +903,10 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
 
   exit_connection_disable:
   	gb_connection_disable(connection);
-exit_release_minor:
-	release_minor(gb_tty);
-exit_kfifo_free:
-	kfifo_free(&gb_tty->write_fifo);
-exit_buf_free:
-	kfree(gb_tty->buffer);
+exit_put_port:
+	tty_port_put(&gb_tty->port);
   exit_connection_destroy:
   	gb_connection_destroy(connection);
-exit_tty_free:
-	kfree(gb_tty);
 
   	return retval;
   }

@@ -930,15 +937,10 @@ static void gb_uart_remove(struct gbphy_device *gbphy_dev)
   	gb_connection_disable_rx(connection);
   	tty_unregister_device(gb_tty_driver, gb_tty->minor);
 
-	/* FIXME - free transmit / receive buffers */
-
   	gb_connection_disable(connection);
-	tty_port_destroy(&gb_tty->port);
   	gb_connection_destroy(connection);
-	release_minor(gb_tty);
-	kfifo_free(&gb_tty->write_fifo);
-	kfree(gb_tty->buffer);
-	kfree(gb_tty);
+

Quoted 8 lines

Review static void gb_uart_remove(struct gbphy_device *gbphy_dev) · line 943
Quoted 1 line
(quoted diff) +1 -0
+	tty_port_put(&gb_tty->port);
In the error path above, you call tty_port_put() before calling gb_connection_destroy(), which matches (in reverse) the order in which they're created. I'm accustomed to having the order of the calls here match the error path. Is this difference intentional? (It shouldn't really matter.)

Like you say, the order here doesn't really matter so I can move it up if you prefer.

Johan