[PATCH v2 0/1] mcb: Fix crash mcb-core module is removed

[PATCH v2 0/1] mcb: Fix crash mcb-core module is removed

[Rodríguez Barbarin, José Javier]

When allocating a new mcb_bus the bus_type is added to the mcb_bus itself,
causing an issue when calling mcb_bus_add_devices(). This function is not
only called for each mcb_device under the mcb_bus but for the bus itself.

The crash happens when the mcb_core module is removed, getting
the following error:

[  286.691693] ------------[ cut here ]------------
[  286.691695] ida_free called for id=1 which is not allocated.
[  286.691714] WARNING: CPU: 0 PID: 1719 at lib/idr.c:523 ida_free+0xe0/0x140
[  286.691715] Modules linked in: snd_hda_codec_hdmi amd64_edac_mod snd_hda_intel edac_mce_amd snd_intel_dspcfg kvm_amd snd_hda_codec amdgpu nls_iso8859_1 ccp snd_hda_core snd_hwdep amd_iommu_v2 kvm snd_pcm gpu_sched crct10dif_pclmul crc32_pclmul snd_seq_midi snd_seq_midi_event ghash_clmulni_intel ttm snd_rawmidi aesni_intel snd_seq binfmt_misc crypto_simd cryptd glue_helper drm_kms_helper snd_seq_device snd_timer drm snd k10temp fb_sys_fops syscopyarea sysfillrect sysimgblt snd_rn_pci_acp3x mcb_pci(-) snd_pci_acp3x soundcore altera_cvp fpga_mgr mcb spi_nor mtd 8250_dw mac_hid sch_fq_codel parport_pc ppdev lp parport ip_tables x_tables autofs4 mmc_block nvme ahci i2c_piix4 libahci i2c_amd_mp2_pci igb nvme_core i2c_algo_bit dca video sdhci_acpi sdhci [last unloaded: 8250_men_mcb]
[  286.691752] CPU: 0 PID: 1719 Comm: modprobe Not tainted 5.4.702+ #11
[  286.691753] Hardware name: MEN F027/n/a, BIOS 1.03 04/20/2021
[  286.691756] RIP: 0010:ida_free+0xe0/0x140
[  286.691759] Code: a8 31 f6 e8 12 f7 00 00 eb 4b 4c 0f a3 28 72 21 48 8b 7d a8 4c 89 f6 e8 8e ad 02 00 89 de 48 c7 c7 e8 02 83 b5 e8 b0 7a 5d ff <0f> 0b e9 67 ff ff ff 4c 0f b3 28 48 8d 7d a8 31 f6 e8 da e0 00 00
[  286.691761] RSP: 0018:ffff9a56c38f7bd8 EFLAGS: 00010282
[  286.691763] RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000006
[  286.691764] RDX: 0000000000000007 RSI: 0000000000000096 RDI: ffff8d881fa1c8c0
[  286.691765] RBP: ffff9a56c38f7c30 R08: 0000000000000487 R09: 0000000000000004
[  286.691766] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000001
[  286.691767] R13: 0000000000000001 R14: 0000000000000202 R15: 0000000000000001
[  286.691769] FS:  00007fb78e303540(0000) GS:ffff8d881fa00000(0000) knlGS:0000000000000000
[  286.691770] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  286.691771] CR2: 00007ffe92b2ce98 CR3: 000000079fd9c000 CR4: 00000000003406f0
[  286.691772] Call Trace:
[  286.691781]  mcb_free_bus+0x2b/0x40 [mcb]
[  286.691785]  device_release+0x2c/0x80
[  286.691787]  kobject_put+0xb9/0x1d0
[  286.691790]  put_device+0x13/0x20

As mcb_bus_add_devices() is called for the mcb_bus itself, the function
tries to cast the incorrectly passed struct mcb_bus to mcb_device. Both
structs have the same layout:

struct mcb_bus {
        struct device dev;
        struct device *carrier;
        int bus_nr;
...
};

struct mcb_device {
        struct device dev;
        struct mcb_bus *bus;
        bool is_added;
...
};

This incorrect casting is causing a wrong behaviour in
mcb_bus_add_devices() where the member bus_nr is casted to is_added,
meaning that when bus_nr is "0", the function continues and sets bus_nr
to "1" (is_added = true)

If we have 2 buses (one for each F215 board), the function ida_alloc()
will give the value "0" and "1" to each bus respectively, but as both
buses are included themselves in the devices' lists, after the call to
mcb_bus_add_devices(), the buses will have the value "1" and "1". For
this reason, when the mcb-core module is removed, the error raises as
the ida resource with value "1" is being released twice, leaking
the ida resource with value "0".

changes for V2:
* create a dedicated bus_type for mcb_bus and mcb_device structs
  instead of removing bus_type for mcb_bus.

This patch is based on linux-next (next-20230817)

Jose Javier Rodriguez Barbarin (1):
  mcb: create dedicated bus_type for mcb_bus and mcb_device

 drivers/mcb/mcb-core.c | 43 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v2 1/1] mcb: create dedicated bus_type for mcb_bus and mcb_device

[Rodríguez Barbarin, José Javier]

When allocating a new mcb_bus the bus_type is added to the mcb_bus
itself, causing an issue when calling mcb_bus_add_devices().
This function is not only called for each mcb_device under the
mcb_bus but for the bus itself.

This causes a crash when freeing the ida resources as the bus numbering
gets corrupted due to a wrong cast of structs mcb_bus and mcb_device.

Make the release of the mcb devices and their mcb bus explicit.

Fixes: 18d288198099 ("mcb: Correctly initialize the bus's device")
Co-developed-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Jose Javier Rodriguez Barbarin <JoseJavier.Rodriguez@duagon.com>
---
 drivers/mcb/mcb-core.c | 43 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/mcb/mcb-core.c b/drivers/mcb/mcb-core.c
index 978fdfc19a06..d2ac2a12b737 100644
--- a/drivers/mcb/mcb-core.c
+++ b/drivers/mcb/mcb-core.c
@@ -156,7 +156,7 @@ static const struct attribute_group *mcb_carrier_groups[] = {
 };
 
 
-static struct bus_type mcb_bus_type = {
+static struct bus_type mcb_device_type = {
 	.name = "mcb",
 	.match = mcb_match,
 	.uevent = mcb_uevent,
@@ -165,6 +165,11 @@ static struct bus_type mcb_bus_type = {
 	.shutdown = mcb_shutdown,
 };
 
+static struct bus_type mcb_bus_type = {
+	.name = "mcb-bus",
+	.dev_name = "mcb-bus",
+};
+
 static struct device_type mcb_carrier_device_type = {
 	.name = "mcb-carrier",
 	.groups = mcb_carrier_groups,
@@ -186,7 +191,7 @@ int __mcb_register_driver(struct mcb_driver *drv, struct module *owner,
 		return -EINVAL;
 
 	drv->driver.owner = owner;
-	drv->driver.bus = &mcb_bus_type;
+	drv->driver.bus = &mcb_device_type;
 	drv->driver.mod_name = mod_name;
 
 	return driver_register(&drv->driver);
@@ -227,7 +232,7 @@ int mcb_device_register(struct mcb_bus *bus, struct mcb_device *dev)
 
 	device_initialize(&dev->dev);
 	mcb_bus_get(bus);
-	dev->dev.bus = &mcb_bus_type;
+	dev->dev.bus = &mcb_device_type;
 	dev->dev.parent = bus->dev.parent;
 	dev->dev.release = mcb_release_dev;
 	dev->dma_dev = bus->carrier;
@@ -251,6 +256,12 @@ int mcb_device_register(struct mcb_bus *bus, struct mcb_device *dev)
 }
 EXPORT_SYMBOL_NS_GPL(mcb_device_register, MCB);
 
+
+static void mcb_bus_unregister(struct mcb_bus *bus)
+{
+	device_unregister(&bus->dev);
+}
+
 static void mcb_free_bus(struct device *dev)
 {
 	struct mcb_bus *bus = to_mcb_bus(dev);
@@ -311,7 +322,7 @@ static int __mcb_devices_unregister(struct device *dev, void *data)
 
 static void mcb_devices_unregister(struct mcb_bus *bus)
 {
-	bus_for_each_dev(&mcb_bus_type, NULL, NULL, __mcb_devices_unregister);
+	bus_for_each_dev(&mcb_device_type, NULL, NULL, __mcb_devices_unregister);
 }
 /**
  * mcb_release_bus() - Free a @mcb_bus
@@ -322,6 +333,7 @@ static void mcb_devices_unregister(struct mcb_bus *bus)
 void mcb_release_bus(struct mcb_bus *bus)
 {
 	mcb_devices_unregister(bus);
+	mcb_bus_unregister(bus);
 }
 EXPORT_SYMBOL_NS_GPL(mcb_release_bus, MCB);
 
@@ -410,7 +422,7 @@ static int __mcb_bus_add_devices(struct device *dev, void *data)
  */
 void mcb_bus_add_devices(const struct mcb_bus *bus)
 {
-	bus_for_each_dev(&mcb_bus_type, NULL, NULL, __mcb_bus_add_devices);
+	bus_for_each_dev(&mcb_device_type, NULL, NULL, __mcb_bus_add_devices);
 }
 EXPORT_SYMBOL_NS_GPL(mcb_bus_add_devices, MCB);
 
@@ -499,12 +511,31 @@ EXPORT_SYMBOL_NS_GPL(mcb_get_irq, MCB);
 
 static int mcb_init(void)
 {
-	return bus_register(&mcb_bus_type);
+	int res;
+
+	res = bus_register(&mcb_bus_type);
+
+	if (res < 0)
+		goto mcb_bus_register_fail;
+
+	res = bus_register(&mcb_device_type);
+
+	if (res < 0)
+		goto mcb_device_register_fail;
+
+	return 0;
+
+mcb_device_register_fail:
+	bus_unregister(&mcb_bus_type);
+
+mcb_bus_register_fail:
+	return res;
 }
 
 static void mcb_exit(void)
 {
 	ida_destroy(&mcb_ida);
+	bus_unregister(&mcb_device_type);
 	bus_unregister(&mcb_bus_type);
 }
 
-- 
2.34.1

Re: [PATCH v2 1/1] mcb: create dedicated bus_type for mcb_bus and mcb_device

[gregkh]

On Fri, Aug 18, 2023 at 11:03:03AM +0000, Rodríguez Barbarin, José Javier wrote:
> When allocating a new mcb_bus the bus_type is added to the mcb_bus
> itself, causing an issue when calling mcb_bus_add_devices().
> This function is not only called for each mcb_device under the
> mcb_bus but for the bus itself.
> 
> This causes a crash when freeing the ida resources as the bus numbering
> gets corrupted due to a wrong cast of structs mcb_bus and mcb_device.

Why not just fix this up and determine the "type" of the device before
you cast anything?

> Make the release of the mcb devices and their mcb bus explicit.

That's good, but now you have a new bus type which is a bit odd just for
the bus controller.  It's not necessarily bad, but not generally what
other busses do.  As an example, USB controllers are not their own bus
type, but rather, a different type of device on the same bus.  Same goes
for greybus devices/controllers.

So perhaps try doing that instead of creating a whole new bus here?

thanks,

greg k-h

Re: [PATCH v2 1/1] mcb: create dedicated bus_type for mcb_bus and mcb_device

[Sanjuán García, Jorge]

On Sat, 2023-08-19 at 11:56 +0200, gregkh@linuxfoundation.org wrote:
> On Fri, Aug 18, 2023 at 11:03:03AM +0000, Rodríguez Barbarin, José
> Javier wrote:
> > When allocating a new mcb_bus the bus_type is added to the mcb_bus
> > itself, causing an issue when calling mcb_bus_add_devices().
> > This function is not only called for each mcb_device under the
> > mcb_bus but for the bus itself.
> > 
> > This causes a crash when freeing the ida resources as the bus
> > numbering
> > gets corrupted due to a wrong cast of structs mcb_bus and
> > mcb_device.
> 
> Why not just fix this up and determine the "type" of the device
> before
> you cast anything?

Hi Greg,

Thanks for the review.

We will try and fix the root cause of this. The only reason we
need this cast for mcb_device struct in __mcb_bus_add_devices()
is to update an is_added flag to track whether the device has
already been attached to a driver or not. However, this is
already reported in the return value of device_attach() so we
may avoid the casting issues by just not casting to any mcb_foo 
struct there and just use the device struct.

We will try this approach and send a V3 patchset for fixing this
crash when releasing mcb devices.

Regards,
Jorge

> 
> > Make the release of the mcb devices and their mcb bus explicit.
> 
> That's good, but now you have a new bus type which is a bit odd just
> for
> the bus controller.  It's not necessarily bad, but not generally what
> other busses do.  As an example, USB controllers are not their own
> bus
> type, but rather, a different type of device on the same bus.  Same
> goes
> for greybus devices/controllers.
> 
> So perhaps try doing that instead of creating a whole new bus here?
> 
> thanks,
> 
> greg k-h