Skip to contentSkip to conversation

panic in sg_add when class_device_create() fails

DISCUSSION Michael Reed 3 participants 3 messages Jan 14, 2008 – Jan 15, 2008

Subsystems (2)
SCSI SCSI SG driver
lore
Share & tools
lore

Michael Reed

author
#0 lore raw

We're seeing an occasional panic in sg_add() when class_device_create() fails. It's obvious in the code that it uses the pointer to sg_class_member even though it's invalid. We do see the "class_device_create failed" message.

        class_set_devdata(cl_dev, sdp);
        error = cdev_add(cdev, MKDEV(SCSI_GENERIC_MAJOR, sdp->index), 1);
        if (error)
                goto cdev_add_err;

        if (sg_sysfs_valid) {
                struct class_device * sg_class_member;

                sg_class_member = class_device_create(sg_sysfs_class, NULL,
                                MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
                                cl_dev->dev, "%s",
                                disk->disk_name);
                if (IS_ERR(sg_class_member))
                        printk(KERN_WARNING "sg_add: "
                                "class_device_create failed\n");
                class_set_devdata(sg_class_member, sdp);
                                  ^^^^^^^^^^^^^^^^
                error = sysfs_create_link(&scsidp->sdev_gendev.kobj,
                                          &sg_class_member->kobj, "generic");
                if (error)
                        printk(KERN_ERR "sg_add: unable to make symlink "
                                        "'generic' back to sg%d\n", sdp->index);
        } else
                printk(KERN_WARNING "sg_add: sg_sys Invalid\n");

I'm uncertain of the correct fix. Perhaps it involves a call to cdev_unmap()? I don't have a good way to test a fix as this problem is not easily reproduced.

Mike

maintainer
#1 lore raw
Quoted 29 lines · Michael Reed

We're seeing an occasional panic in sg_add() when class_device_create() fails. It's obvious in the code that it uses the pointer to sg_class_member even though it's invalid. We do see the "class_device_create failed" message.

        class_set_devdata(cl_dev, sdp);
        error = cdev_add(cdev, MKDEV(SCSI_GENERIC_MAJOR, sdp->index), 1);
        if (error)
                goto cdev_add_err;

        if (sg_sysfs_valid) {
                struct class_device * sg_class_member;

                sg_class_member = class_device_create(sg_sysfs_class, NULL,
                                MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
                                cl_dev->dev, "%s",
                                disk->disk_name);
                if (IS_ERR(sg_class_member))
                        printk(KERN_WARNING "sg_add: "
                                "class_device_create failed\n");
                class_set_devdata(sg_class_member, sdp);
                                  ^^^^^^^^^^^^^^^^
                error = sysfs_create_link(&scsidp->sdev_gendev.kobj,
                                          &sg_class_member->kobj, "generic");
                if (error)
                        printk(KERN_ERR "sg_add: unable to make symlink "
                                        "'generic' back to sg%d\n", sdp->index);
        } else
                printk(KERN_WARNING "sg_add: sg_sys Invalid\n");
Quoted 2 lines

I'm uncertain of the correct fix. Perhaps it involves a call to cdev_unmap()? I don't have a good way to test a fix as this problem is not easily reproduced.

I think just

error = ERR_PTR(sg_class_member);
goto cdev_add_err;

Will do it, won't it?

James

#2 lore raw
In reply to Michael Reed
Quoted 29 lines · On Mon, 14 Jan 2008 14:11:36 -0800 Michael Reed <[email protected]> wrote:

We're seeing an occasional panic in sg_add() when class_device_create() fails. It's obvious in the code that it uses the pointer to sg_class_member even though it's invalid. We do see the "class_device_create failed" message.

        class_set_devdata(cl_dev, sdp);
        error = cdev_add(cdev, MKDEV(SCSI_GENERIC_MAJOR, sdp->index), 1);
        if (error)
                goto cdev_add_err;

        if (sg_sysfs_valid) {
                struct class_device * sg_class_member;

                sg_class_member = class_device_create(sg_sysfs_class, NULL,
                                MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
                                cl_dev->dev, "%s",
                                disk->disk_name);
                if (IS_ERR(sg_class_member))
                        printk(KERN_WARNING "sg_add: "
                                "class_device_create failed\n");
                class_set_devdata(sg_class_member, sdp);
                                  ^^^^^^^^^^^^^^^^
                error = sysfs_create_link(&scsidp->sdev_gendev.kobj,
                                          &sg_class_member->kobj, "generic");
                if (error)
                        printk(KERN_ERR "sg_add: unable to make symlink "
                                        "'generic' back to sg%d\n", sdp->index);
        } else
                printk(KERN_WARNING "sg_add: sg_sys Invalid\n");
Quoted 1 line

I'm uncertain of the correct fix. Perhaps it involves a call to cdev_unmap()?

The following patches work for me:

http://marc.info/?l=linux-scsi&m=120037070303939&w=2
http://marc.info/?l=linux-scsi&m=120037070303941&w=2

Quoted 2 lines

I don't have a good way to test a fix as this problem is not easily reproduced.

I used the attached patch (though the fault injection feature can do something better, I guess).

drivers/scsi/sg.c +8 -4
index 527e2eb..4cdd213 100644
@@ -1424,12 +1424,16 @@ sg_add(struct class_device *cl_dev, struct class_interface *cl_intf)
 
 	sdp->cdev = cdev;
 	if (sg_sysfs_valid) {
+		static int count = 0;
 		struct class_device * sg_class_member;
 
-		sg_class_member = class_device_create(sg_sysfs_class, NULL,
-				MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
-				cl_dev->dev, "%s",
-				disk->disk_name);
+		if (++count % 2)
+			sg_class_member = class_device_create(sg_sysfs_class, NULL,
+							      MKDEV(SCSI_GENERIC_MAJOR, sdp->index),
+							      cl_dev->dev, "%s",
+							      disk->disk_name);
+		else
+			sg_class_member = ERR_PTR(-ENOMEM);
 		if (IS_ERR(sg_class_member)) {
 			printk(KERN_ERR "sg_add: "
 			       "class_device_create failed\n");