hso: fix refcnt leak in recent patch.

Prior to
commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
hso: fix crash when device disappears while serial port is open

hso_serial_open would always kref_get(&serial->parent->ref) before returning zero.
Since that commit, it only calls kref_get when returning 0 if serial->port.count was zero.

This results in calls to

       kref_put(&serial->parent->ref, hso_serial_ref_free);

after hso_serial_ref_free has been called, which dereferences a freed pointer.

This patch adds the missing kref_get().

Fixes commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b

Cc [email protected] (v4.0)

Cc Olivier Sobrie <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/net/usb/hso.c +0 -0

index 75befc1bd816..6848fc903340 100644

Quoted 35 lines · On Tue, 14 Apr 2015 09:36:34 +1000 NeilBrown <[email protected]> wrote:

Prior to
commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
hso: fix crash when device disappears while serial port is open

hso_serial_open would always kref_get(&serial->parent->ref) before returning zero.
Since that commit, it only calls kref_get when returning 0 if serial->port.count was zero.

This results in calls to

       kref_put(&serial->parent->ref, hso_serial_ref_free);

after hso_serial_ref_free has been called, which dereferences a freed pointer.

This patch adds the missing kref_get().

Fixes commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b

Cc [email protected] (v4.0)

Cc Olivier Sobrie <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/net/usb/hso.c +1 -0

index 75befc1bd816..6848fc903340 100644
@@ -1299,6 +1299,7 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		}
 	} else {
 		D1("Port was already open");
+		kref_get(&serial->parent->ref);
 	}
 

drivers/net/usb/hso.c static int hso_serial_open(struct tty_struct *tty, struct file *filp) · line 1305

Quoted 1 line

drivers/net/usb/hso.c +0 -0

 	usb_autopm_put_interface(serial->parent->interface);

Sorry - that was wrong.
I'm getting crashes which strongly suggest the kref_put is being called extra times, but I misunderstood the code and was hasty.

Maybe this instead?

Thanks,
NeilBrown

From: NeilBrown <[email protected]>
Date: Tue, 14 Apr 2015 09:33:03 +1000
Subject: [PATCH] hso: fix refcnt leak in recent patch.

Prior to
commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
hso: fix crash when device disappears while serial port is open

a kref_get on serial->parent->ref would be taken on each open, and it would be kref_put on each close.

Now the kref_put happens when the tty_struct is finally put (via the 'cleanup') providing tty->driver_data has been set. So the kref_get must be called exact once when tty->driver_data is set.

With the current code, if the first open fails the kref_get() is never called, but the kref_put() is called, leaving to a crash.

So change the kref_get call to happen exactly when ->driver_data is changed from NULL to non-NULL.

Fixes commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b

Cc [email protected] (v4.0)

Cc Olivier Sobrie <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/net/usb/hso.c +2 -2

index 75befc1bd816..17fd3820263a 100644
@@ -1278,6 +1278,8 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 	D1("Opening %d", serial->minor);
 
 	/* setup */
+	if (tty->driver_data == NULL)
+		kref_get(&serial->parent->ref);
 	tty->driver_data = serial;
 	tty_port_tty_set(&serial->port, tty);
 

@@ -1294,8 +1296,6 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		if (result) {
 			hso_stop_serial_device(serial->parent);
 			serial->port.count--;
-		} else {
-			kref_get(&serial->parent->ref);
 		}
 	} else {
 		D1("Port was already open");

Hello Neil,

Quoted 44 lines · NeilBrown

Quoted 29 lines · On Tue, 14 Apr 2015 09:36:34 +1000 NeilBrown <[email protected]> wrote:

Prior to
commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
hso: fix crash when device disappears while serial port is open

hso_serial_open would always kref_get(&serial->parent->ref) before returning zero.
Since that commit, it only calls kref_get when returning 0 if serial->port.count was zero.

This results in calls to

       kref_put(&serial->parent->ref, hso_serial_ref_free);

after hso_serial_ref_free has been called, which dereferences a freed pointer.

This patch adds the missing kref_get().

Fixes commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b

Cc [email protected] (v4.0)

Cc Olivier Sobrie <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/net/usb/hso.c +0 -0

index 75befc1bd816..6848fc903340 100644

drivers/net/usb/hso.c static int hso_serial_open(struct tty_struct *tty, struct file *filp) · line 1305

Quoted 7 lines

drivers/net/usb/hso.c +1 -0

@@ -1299,6 +1299,7 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		}
 	} else {
 		D1("Port was already open");
+		kref_get(&serial->parent->ref);
 	}
 
 	usb_autopm_put_interface(serial->parent->interface);

Sorry - that was wrong.
I'm getting crashes which strongly suggest the kref_put is being called extra times, but I misunderstood the code and was hasty.

Quoted 1 line

Maybe this instead?

Indeed, if I undestand correctly the code in tty_io.c, cleanup() method is also called when the open fails while kref_get is only done if the open succeeds. Sorry for that mess.
I assume you get that crash when hso_start_serial_device() returns an error?

At first sight, the patch below looks good to me.
I'll test it in the next days.

Thank you,

Olivier

Quoted 45 lines

Thanks,
NeilBrown

From: NeilBrown <[email protected]>
Date: Tue, 14 Apr 2015 09:33:03 +1000
Subject: [PATCH] hso: fix refcnt leak in recent patch.

Prior to
commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
hso: fix crash when device disappears while serial port is open

a kref_get on serial->parent->ref would be taken on each open, and it would be kref_put on each close.

Now the kref_put happens when the tty_struct is finally put (via the 'cleanup') providing tty->driver_data has been set. So the kref_get must be called exact once when tty->driver_data is set.

With the current code, if the first open fails the kref_get() is never called, but the kref_put() is called, leaving to a crash.

So change the kref_get call to happen exactly when ->driver_data is changed from NULL to non-NULL.

Fixes commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b

Cc [email protected] (v4.0)

Cc Olivier Sobrie <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/net/usb/hso.c +2 -0

index 75befc1bd816..17fd3820263a 100644
@@ -1278,6 +1278,8 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 	D1("Opening %d", serial->minor);
 
 	/* setup */
+	if (tty->driver_data == NULL)
+		kref_get(&serial->parent->ref);
 	tty->driver_data = serial;
 	tty_port_tty_set(&serial->port, tty);
 

drivers/net/usb/hso.c static int hso_serial_open(struct tty_struct *tty, struct file *filp) · line 1301

Quoted 8 lines

drivers/net/usb/hso.c +0 -2

@@ -1294,8 +1296,6 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		if (result) {
 			hso_stop_serial_device(serial->parent);
 			serial->port.count--;
-		} else {
-			kref_get(&serial->parent->ref);
 		}
 	} else {
 		D1("Port was already open");

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 35 lines · On Tue, 14 Apr 2015 09:36:34 +1000 NeilBrown <[email protected]> wrote:

Prior to
commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
hso: fix crash when device disappears while serial port is open

hso_serial_open would always kref_get(&serial->parent->ref) before returning zero.
Since that commit, it only calls kref_get when returning 0 if serial->port.count was zero.

This results in calls to

       kref_put(&serial->parent->ref, hso_serial_ref_free);

after hso_serial_ref_free has been called, which dereferences a freed pointer.

This patch adds the missing kref_get().

Fixes commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b

Cc [email protected] (v4.0)

Cc Olivier Sobrie <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/net/usb/hso.c +1 -0

index 75befc1bd816..6848fc903340 100644
@@ -1299,6 +1299,7 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		}
 	} else {
 		D1("Port was already open");
+		kref_get(&serial->parent->ref);
 	}
 

drivers/net/usb/hso.c static int hso_serial_open(struct tty_struct *tty, struct file *filp) · line 1305

Quoted 1 line

drivers/net/usb/hso.c +0 -0

 	usb_autopm_put_interface(serial->parent->interface);

Sorry - that was wrong.
I'm getting crashes which strongly suggest the kref_put is being called extra times, but I misunderstood the code and was hasty.

Maybe this instead?

Thanks,
NeilBrown

From: NeilBrown <[email protected]>
Date: Tue, 14 Apr 2015 09:33:03 +1000
Subject: [PATCH] hso: fix refcnt leak in recent patch.

Prior to
commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b
hso: fix crash when device disappears while serial port is open

a kref_get on serial->parent->ref would be taken on each open, and it would be kref_put on each close.

Now the kref_put happens when the tty_struct is finally put (via the 'cleanup') providing tty->driver_data has been set. So the kref_get must be called exact once when tty->driver_data is set.

With the current code, if the first open fails the kref_get() is never called, but the kref_put() is called, leaving to a crash.

So change the kref_get call to happen exactly when ->driver_data is changed from NULL to non-NULL.

Fixes commit 29bd3bc1194c624ce863cab2a7da9bc1f0c3b47b

Cc [email protected] (v4.0)

Cc Olivier Sobrie <[email protected]>

Signed-off-by NeilBrown <[email protected]>

drivers/net/usb/hso.c +2 -2

index 75befc1bd816..17fd3820263a 100644
@@ -1278,6 +1278,8 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 	D1("Opening %d", serial->minor);
 
 	/* setup */
+	if (tty->driver_data == NULL)
+		kref_get(&serial->parent->ref);
 	tty->driver_data = serial;
 	tty_port_tty_set(&serial->port, tty);
 

@@ -1294,8 +1296,6 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		if (result) {
 			hso_stop_serial_device(serial->parent);
 			serial->port.count--;
-		} else {
-			kref_get(&serial->parent->ref);
 		}
 	} else {
 		D1("Port was already open");