[PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Al Viro]

	device_create() will create a symlink in /sys/dev/char unless
the class has ->dev_kobj cleared; pktcdvd uses it to populate the
/sys/class/pktcdvd/ with per-device subdirectories.  While we do
want /sys/class/pktcdvd/pktcdvd[0-7]/dev to contain a device number,
we definitely do not want it to try and crap into /sys/dev/char; if
nothing else, device number is *block* one (and we don't want it
to crap into /sys/dev/block either - there such symlinks will be created
by add_disk() and they will point to /sys/block/pktcdvd[0-7]).

	As it is, attempt to set the damn thing up will end up with
sysfs_add_one() barfing at duplicate entries and /sys/class/pktcdvd/pktcdvd*/*/
not created at all.  The fix is trivial: clear ->dev_kobj in the class to
tell device_add() that no, we do *not* want these symlinks, TYVM...

	FWIW, all traces containing pkt_setup_dev() on kerneloops.org appear
to be from that one.  It had been around since 2.6.26; probably -stable
fodder...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index f20bf35..79981f2 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -417,6 +417,7 @@ static int pkt_sysfs_init(void)
 		printk(DRIVER_NAME": failed to create class pktcdvd\n");
 		return ret;
 	}
+	class_pktcdvd->dev_kobj = NULL;
 	return 0;
 }
 

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Kay Sievers]

On Sun, Nov 30, 2008 at 13:19, Al Viro <viro@zeniv.linux.org.uk> wrote:
>        device_create() will create a symlink in /sys/dev/char unless
> the class has ->dev_kobj cleared; pktcdvd uses it to populate the
> /sys/class/pktcdvd/ with per-device subdirectories.  While we do
> want /sys/class/pktcdvd/pktcdvd[0-7]/dev to contain a device number,
> we definitely do not want it to try and crap into /sys/dev/char; if
> nothing else, device number is *block* one (and we don't want it
> to crap into /sys/dev/block either - there such symlinks will be created
> by add_disk() and they will point to /sys/block/pktcdvd[0-7]).
>
>        As it is, attempt to set the damn thing up will end up with
> sysfs_add_one() barfing at duplicate entries and /sys/class/pktcdvd/pktcdvd*/*/
> not created at all.  The fix is trivial: clear ->dev_kobj in the class to
> tell device_add() that no, we do *not* want these symlinks, TYVM...
>
>        FWIW, all traces containing pkt_setup_dev() on kerneloops.org appear
> to be from that one.  It had been around since 2.6.26; probably -stable
> fodder...
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
> index f20bf35..79981f2 100644
> --- a/drivers/block/pktcdvd.c
> +++ b/drivers/block/pktcdvd.c
> @@ -417,6 +417,7 @@ static int pkt_sysfs_init(void)
>                printk(DRIVER_NAME": failed to create class pktcdvd\n");
>                return ret;
>        }
> +       class_pktcdvd->dev_kobj = NULL;
>        return 0;
>  }

I posted a fix for that weeks ago. But the pktcdvd maintainer stated,
that the char device nodes are not used for anything. So the whole use
of dev_t should be removed entirely. They just blindly claim the same
char dev_t the block devices use, and conflict with char devices from
other subsystems. Patching out the /sys/dev/class links fixes the
oops, but the underlying fundamental breakage will still exist.

Original mail here states:
"Maybe, but that character device would not be used for anything,
besides creating sub-directories in /sys/class/pktcdvd. The driver
implements a block device, not a character device."

http://lkml.org/lkml/2008/10/14/318

No idea why this never got fixed.

Thanks,
Kay

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Al Viro]

On Sun, Nov 30, 2008 at 01:41:29PM +0100, Kay Sievers wrote:

> I posted a fix for that weeks ago. But the pktcdvd maintainer stated,
> that the char device nodes are not used for anything. So the whole use
> of dev_t should be removed entirely. They just blindly claim the same
> char dev_t the block devices use, and conflict with char devices from
> other subsystems. Patching out the /sys/dev/class links fixes the
> oops, but the underlying fundamental breakage will still exist.
> 
> Original mail here states:
> "Maybe, but that character device would not be used for anything,
> besides creating sub-directories in /sys/class/pktcdvd. The driver
> implements a block device, not a character device."

_What_ character device nodes?  The only thing that dev_t value used to
be used for was giving contents for a text file - ..../dev in sysfs directory.  
With introduction of /sys/dev/char the damn thing got confused for
character device number *by* *device_create()*.  That's all.

There are no character device nodes at all.  Driver doesn't claim them.
Not in that call of device_create(), not anywhere else.  sysfs doesn't
create them (or any other device nodes).  If you mknod such thing as
char device and try to open it, the kernel won't go anywhere near this driver,
whether there'd been other drivers or not. 

So what kind of fundamental breakage are you talking about?  All I can see
here is a bogus entry added to bogu^Wmisguided^W /sys/dev/char, which leads
to WARN_ON() in case if somebody had char device with device number numerically
equal to that of our block device and to silent crapping into /sys/dev/char
if no such char device had been there already.

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Kay Sievers]

On Sun, Nov 30, 2008 at 14:21, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Nov 30, 2008 at 01:41:29PM +0100, Kay Sievers wrote:
>
>> I posted a fix for that weeks ago. But the pktcdvd maintainer stated,
>> that the char device nodes are not used for anything. So the whole use
>> of dev_t should be removed entirely. They just blindly claim the same
>> char dev_t the block devices use, and conflict with char devices from
>> other subsystems. Patching out the /sys/dev/class links fixes the
>> oops, but the underlying fundamental breakage will still exist.
>>
>> Original mail here states:
>> "Maybe, but that character device would not be used for anything,
>> besides creating sub-directories in /sys/class/pktcdvd. The driver
>> implements a block device, not a character device."
>
> _What_ character device nodes?  The only thing that dev_t value used to
> be used for was giving contents for a text file - ..../dev in sysfs directory.
> With introduction of /sys/dev/char the damn thing got confused for
> character device number *by* *device_create()*.  That's all.
>
> There are no character device nodes at all.  Driver doesn't claim them.
> Not in that call of device_create(), not anywhere else.  sysfs doesn't
> create them (or any other device nodes).  If you mknod such thing as
> char device and try to open it, the kernel won't go anywhere near this driver,
> whether there'd been other drivers or not.
>
> So what kind of fundamental breakage are you talking about?  All I can see
> here is a bogus entry added to bogu^Wmisguided^W /sys/dev/char, which leads
> to WARN_ON() in case if somebody had char device with device number numerically
> equal to that of our block device and to silent crapping into /sys/dev/char
> if no such char device had been there already

Pktcdvd creates char _and_ block device nodes at the same time, while
the char nodes are not allocated, but created and conflict in
/sys/dev/ with properly allocated ones from other subsystems. Your
patch just papers over this bug.

Kay

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Al Viro]

On Sun, Nov 30, 2008 at 02:25:43PM +0100, Kay Sievers wrote:

> Pktcdvd creates char _and_ block device nodes at the same time, while
> the char nodes are not allocated, but created and conflict in
> /sys/dev/ with properly allocated ones from other subsystems. Your
> patch just papers over this bug.

Where the hell is it creating any char device nodes?  Show me.
And note that device_create() is not creating *any* device nodes,
char or block.  It creates a directory in some place in sysfs,
associated with struct device (which has arseloads of uses unrelated
to block *or* char devices) and, if dev_t argument is non-zero,
dumps its value into one of the attributes.

These days it might also dump a symlink into /sys/dev/*.

Which part of the above constitutes a creation of char device node?

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Al Viro]

On Sun, Nov 30, 2008 at 01:32:29PM +0000, Al Viro wrote:
> On Sun, Nov 30, 2008 at 02:25:43PM +0100, Kay Sievers wrote:
> 
> > Pktcdvd creates char _and_ block device nodes at the same time, while
> > the char nodes are not allocated, but created and conflict in
> > /sys/dev/ with properly allocated ones from other subsystems. Your
> > patch just papers over this bug.
> 
> Where the hell is it creating any char device nodes?  Show me.
> And note that device_create() is not creating *any* device nodes,
> char or block.  It creates a directory in some place in sysfs,
> associated with struct device (which has arseloads of uses unrelated
> to block *or* char devices) and, if dev_t argument is non-zero,
> dumps its value into one of the attributes.
> 
> These days it might also dump a symlink into /sys/dev/*.
> 
> Which part of the above constitutes a creation of char device node?

Wait a minute...  Are you saying that something in userland ends up
seeing that sucker, noticing .../dev and proceeding to do mknod?

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Kay Sievers]

On Sun, Nov 30, 2008 at 14:40, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Nov 30, 2008 at 01:32:29PM +0000, Al Viro wrote:
>> On Sun, Nov 30, 2008 at 02:25:43PM +0100, Kay Sievers wrote:
>>
>> > Pktcdvd creates char _and_ block device nodes at the same time, while
>> > the char nodes are not allocated, but created and conflict in
>> > /sys/dev/ with properly allocated ones from other subsystems. Your
>> > patch just papers over this bug.
>>
>> Where the hell is it creating any char device nodes?  Show me.
>> And note that device_create() is not creating *any* device nodes,
>> char or block.  It creates a directory in some place in sysfs,
>> associated with struct device (which has arseloads of uses unrelated
>> to block *or* char devices) and, if dev_t argument is non-zero,
>> dumps its value into one of the attributes.
>>
>> These days it might also dump a symlink into /sys/dev/*.
>>
>> Which part of the above constitutes a creation of char device node?
>
> Wait a minute...  Are you saying that something in userland ends up
> seeing that sucker, noticing .../dev and proceeding to do mknod?

Sure, it does mkod, as the kernel says it should. But that does not
cause any real problem.

These device nodes are not used for anything. Opening the created char
pktcdvd device node in userspace would in many cases just open a
random usb device. :)

Kay

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Al Viro]

On Sun, Nov 30, 2008 at 02:44:24PM +0100, Kay Sievers wrote:

> >> Which part of the above constitutes a creation of char device node?
> >
> > Wait a minute...  Are you saying that something in userland ends up
> > seeing that sucker, noticing .../dev and proceeding to do mknod?
> 
> Sure, it does mkod, as the kernel says it should. But that does not
> cause any real problem.

"It" being udev, presumably?

What a mess...  How does drivers/usb/core/devio.c avoid essentially the same
problem?

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Kay Sievers]

On Sun, Nov 30, 2008 at 14:50, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Nov 30, 2008 at 02:44:24PM +0100, Kay Sievers wrote:
>
>> >> Which part of the above constitutes a creation of char device node?
>> >
>> > Wait a minute...  Are you saying that something in userland ends up
>> > seeing that sucker, noticing .../dev and proceeding to do mknod?
>>
>> Sure, it does mkod, as the kernel says it should. But that does not
>> cause any real problem.
>
> "It" being udev, presumably?

Yes, udev, mdev, and a few other tools people use to populate /dev
from the kernel supplied device information.

> What a mess...  How does drivers/usb/core/devio.c avoid essentially the same
> problem?

It's a special case, where two "struct device" have the same dev_t,
but they both point to and handle the same device, so it's fine. The
usb_device class is deprecated, no recent distro uses it, and will be
removed some day.

The /sys/dev/ is handled properly by doing:
  usb_classdev_class->dev_kobj = NULL;

Kay

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Al Viro]

On Sun, Nov 30, 2008 at 02:57:14PM +0100, Kay Sievers wrote:
> > "It" being udev, presumably?
> 
> Yes, udev, mdev, and a few other tools people use to populate /dev
> from the kernel supplied device information.
> 
> > What a mess...  How does drivers/usb/core/devio.c avoid essentially the same
> > problem?
> 
> It's a special case, where two "struct device" have the same dev_t,
> but they both point to and handle the same device, so it's fine. The
> usb_device class is deprecated, no recent distro uses it, and will be
> removed some day.

Would that happened to udev as well...  Yeah, I know, but one can dream ;-/

Bloody wonderful.  So we have
	* userland-exposed layout of directory trees created by pktcdvd
	* uevent mess generated by device_create() and *also* userland-exposed

So we need to preserve the layout, with the easiest way probably being "add
one more ktype and use kobject_init_and_add() instead of that device_create()".
Sigh...

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Kay Sievers]

On Sun, Nov 30, 2008 at 15:13, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Nov 30, 2008 at 02:57:14PM +0100, Kay Sievers wrote:
>> > "It" being udev, presumably?
>>
>> Yes, udev, mdev, and a few other tools people use to populate /dev
>> from the kernel supplied device information.
>>
>> > What a mess...  How does drivers/usb/core/devio.c avoid essentially the same
>> > problem?
>>
>> It's a special case, where two "struct device" have the same dev_t,
>> but they both point to and handle the same device, so it's fine. The
>> usb_device class is deprecated, no recent distro uses it, and will be
>> removed some day.
>
> Would that happened to udev as well...  Yeah, I know, but one can dream ;-/

If there are two devices with the same dev_t, and udev is configured
to create the same names in /dev, it will create the node for the
first event, and reuse is for the second identical event. There is no
problem here.

> Bloody wonderful.  So we have
>        * userland-exposed layout of directory trees created by pktcdvd
>        * uevent mess generated by device_create() and *also* userland-exposed
>
> So we need to preserve the layout, with the easiest way probably being "add
> one more ktype and use kobject_init_and_add() instead of that device_create()".
> Sigh...

What do you mean? We just need to replace the bogus "pd->pkt_dev" with
MKDEV(0, 0) and we are fine.

Kay

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Al Viro]

On Sun, Nov 30, 2008 at 03:28:15PM +0100, Kay Sievers wrote:
> > So we need to preserve the layout, with the easiest way probably being "add
> > one more ktype and use kobject_init_and_add() instead of that device_create()".
> > Sigh...
> 
> What do you mean? We just need to replace the bogus "pd->pkt_dev" with
> MKDEV(0, 0) and we are fine.

Userland-visible change - right now cat /sys/class/pktcdvd/pktcdvd3/dev will
give you dev_t of the block device in question.

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Al Viro]

On Sun, Nov 30, 2008 at 02:52:21PM +0000, Al Viro wrote:
> On Sun, Nov 30, 2008 at 03:28:15PM +0100, Kay Sievers wrote:
> > > So we need to preserve the layout, with the easiest way probably being "add
> > > one more ktype and use kobject_init_and_add() instead of that device_create()".
> > > Sigh...
> > 
> > What do you mean? We just need to replace the bogus "pd->pkt_dev" with
> > MKDEV(0, 0) and we are fine.
> 
> Userland-visible change - right now cat /sys/class/pktcdvd/pktcdvd3/dev will
> give you dev_t of the block device in question.

Looking at the locking scheme there, it appears that we have an unpleasant
race of the same kind as discussed in md-with-eviction thread.  If removal
hits between finding (and grabbing) gendisk and actual call of ->open(),
it will succeed just fine and we might get an open for _different_ object,
while holding a reference to disk that had already gone through del_gendisk().
Call ioctl() on that sucker and you've got
        struct pktcdvd_device *pd = bdev->bd_disk->private_data;
which will point to already freed object.

It really appears that we need to
	* add ->use() and ->unuse() callbacks
	* have exact_lock() call ->use() in addition to get_disk() - either
on each hit, or on the "it's currently not in use" ones.
	* have ->unuse() called on failure exits in __blkdev_get() and in
blkdev_put(), with rules matching those for calls of ->use().

That would allow to close that kind of races, both here and in md.  I'd
probably prefer the second variant for calling rules - we would be able
to bracket the "it's associated with underlying objects" intervals by
those calls...  Hell knows; I really need to get some sleep and look through
the ->open() and ->release() instances for block devices.  Later...

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Kay Sievers]

On Sun, Nov 30, 2008 at 15:52, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Nov 30, 2008 at 03:28:15PM +0100, Kay Sievers wrote:
>> > So we need to preserve the layout, with the easiest way probably being "add
>> > one more ktype and use kobject_init_and_add() instead of that device_create()".
>> > Sigh...
>>
>> What do you mean? We just need to replace the bogus "pd->pkt_dev" with
>> MKDEV(0, 0) and we are fine.
>
> Userland-visible change - right now cat /sys/class/pktcdvd/pktcdvd3/dev will
> give you dev_t of the block device in question.

True, it's visible, but it's incorrect dead information which should
be removed. Subsystem != "block is defined as S_IFCHR, and there is no
such char device for pktcdvd. In fact the "dev" file of the pktcdvd
class device points in many cases to a USB device, which must be
fixed.

The whole pktcdvd class is not used at all, besides for exporting a
few files. The "dev" file at the class device is just a bug, and the
MAJOR/MINOR uevent environment variables also. I doubt that there will
be any visible breakage in something that isn't already totally
broken. In short, I doubt, that anything that works today will stop
working if we remove the bugus "dev" file.

This seem like the simple and correct fix for the issue with /sys/dev/
and the broken non-existing but exported pkcdvd char device:
-               pd->dev = device_create(class_pktcdvd, NULL, pd->pkt_dev, NULL,
+               pd->dev = device_create(class_pktcdvd, NULL, MKDEV(0, 0), NULL,

Kay

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Kay Sievers]

On Sun, Nov 30, 2008 at 16:56, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Sun, Nov 30, 2008 at 15:52, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Sun, Nov 30, 2008 at 03:28:15PM +0100, Kay Sievers wrote:
>>> > So we need to preserve the layout, with the easiest way probably being "add
>>> > one more ktype and use kobject_init_and_add() instead of that device_create()".
>>> > Sigh...
>>>
>>> What do you mean? We just need to replace the bogus "pd->pkt_dev" with
>>> MKDEV(0, 0) and we are fine.
>>
>> Userland-visible change - right now cat /sys/class/pktcdvd/pktcdvd3/dev will
>> give you dev_t of the block device in question.
>
> True, it's visible, but it's incorrect dead information which should
> be removed. Subsystem != "block is defined as S_IFCHR, and there is no
> such char device for pktcdvd. In fact the "dev" file of the pktcdvd
> class device points in many cases to a USB device, which must be
> fixed.
>
> The whole pktcdvd class is not used at all, besides for exporting a
> few files. The "dev" file at the class device is just a bug, and the
> MAJOR/MINOR uevent environment variables also. I doubt that there will
> be any visible breakage in something that isn't already totally
> broken. In short, I doubt, that anything that works today will stop
> working if we remove the bugus "dev" file.
>
> This seem like the simple and correct fix for the issue with /sys/dev/
> and the broken non-existing but exported pkcdvd char device:
> -               pd->dev = device_create(class_pktcdvd, NULL, pd->pkt_dev, NULL,
> +               pd->dev = device_create(class_pktcdvd, NULL, MKDEV(0, 0), NULL,

Al, this bug still exists, right? Any objections to remove the dead char dev_t?

Thanks,
Kay

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Kay Sievers]

On Fri, 2008-12-05 at 03:57 +0100, Kay Sievers wrote:
> On Sun, Nov 30, 2008 at 16:56, Kay Sievers <kay.sievers@vrfy.org> wrote:
> > On Sun, Nov 30, 2008 at 15:52, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >> On Sun, Nov 30, 2008 at 03:28:15PM +0100, Kay Sievers wrote:
> >>> > So we need to preserve the layout, with the easiest way probably being "add
> >>> > one more ktype and use kobject_init_and_add() instead of that device_create()".
> >>> > Sigh...
> >>>
> >>> What do you mean? We just need to replace the bogus "pd->pkt_dev" with
> >>> MKDEV(0, 0) and we are fine.
> >>
> >> Userland-visible change - right now cat /sys/class/pktcdvd/pktcdvd3/dev will
> >> give you dev_t of the block device in question.
> >
> > True, it's visible, but it's incorrect dead information which should
> > be removed. Subsystem != "block is defined as S_IFCHR, and there is no
> > such char device for pktcdvd. In fact the "dev" file of the pktcdvd
> > class device points in many cases to a USB device, which must be
> > fixed.
> >
> > The whole pktcdvd class is not used at all, besides for exporting a
> > few files. The "dev" file at the class device is just a bug, and the
> > MAJOR/MINOR uevent environment variables also. I doubt that there will
> > be any visible breakage in something that isn't already totally
> > broken. In short, I doubt, that anything that works today will stop
> > working if we remove the bugus "dev" file.
> >
> > This seem like the simple and correct fix for the issue with /sys/dev/
> > and the broken non-existing but exported pkcdvd char device:
> > -               pd->dev = device_create(class_pktcdvd, NULL, pd->pkt_dev, NULL,
> > +               pd->dev = device_create(class_pktcdvd, NULL, MKDEV(0, 0), NULL,
> 
> Al, this bug still exists, right? Any objections to remove the dead char dev_t?

Here is a patch.

Thanks,
Kay


From: Kay Sievers <kay.sievers@vrfy.org>
Subject: pktcdvd: remove broken dev_t export of class devices

The pktcdvd created class devices only export some sysfs files,
but have no char dev_t registered in the driver.

At class device creation time they copy the dev_t value of the
block device to the char device, wich will register a new char
device in the driver core and userspace, with a conflicting dev_t
value.

In many cases the class devices dev_t just points to a random
USB device. This fixes the sysfs "duplicate entry" errors.

Cc: petero2@telia.com
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---
 pktcdvd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -302,7 +302,7 @@ static struct kobj_type kobj_pkt_type_wqueue = {
 static void pkt_sysfs_dev_new(struct pktcdvd_device *pd)
 {
 	if (class_pktcdvd) {
-		pd->dev = device_create(class_pktcdvd, NULL, pd->pkt_dev, NULL,
+		pd->dev = device_create(class_pktcdvd, NULL, MKDEV(0, 0), NULL,
 					"%s", pd->name);
 		if (IS_ERR(pd->dev))
 			pd->dev = NULL;

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Peter Osterlund]

On Sat, 6 Dec 2008, Kay Sievers wrote:

> On Fri, 2008-12-05 at 03:57 +0100, Kay Sievers wrote:
>> On Sun, Nov 30, 2008 at 16:56, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>> On Sun, Nov 30, 2008 at 15:52, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>>> On Sun, Nov 30, 2008 at 03:28:15PM +0100, Kay Sievers wrote:
>>>>>> So we need to preserve the layout, with the easiest way probably being "add
>>>>>> one more ktype and use kobject_init_and_add() instead of that device_create()".
>>>>>> Sigh...
>>>>>
>>>>> What do you mean? We just need to replace the bogus "pd->pkt_dev" with
>>>>> MKDEV(0, 0) and we are fine.
>>>>
>>>> Userland-visible change - right now cat /sys/class/pktcdvd/pktcdvd3/dev will
>>>> give you dev_t of the block device in question.
>>>
>>> True, it's visible, but it's incorrect dead information which should
>>> be removed. Subsystem != "block is defined as S_IFCHR, and there is no
>>> such char device for pktcdvd. In fact the "dev" file of the pktcdvd
>>> class device points in many cases to a USB device, which must be
>>> fixed.
>>>
>>> The whole pktcdvd class is not used at all, besides for exporting a
>>> few files. The "dev" file at the class device is just a bug, and the
>>> MAJOR/MINOR uevent environment variables also. I doubt that there will
>>> be any visible breakage in something that isn't already totally
>>> broken. In short, I doubt, that anything that works today will stop
>>> working if we remove the bugus "dev" file.
>>>
>>> This seem like the simple and correct fix for the issue with /sys/dev/
>>> and the broken non-existing but exported pkcdvd char device:
>>> -               pd->dev = device_create(class_pktcdvd, NULL, pd->pkt_dev, NULL,
>>> +               pd->dev = device_create(class_pktcdvd, NULL, MKDEV(0, 0), NULL,
>>
>> Al, this bug still exists, right? Any objections to remove the dead char dev_t?
>
> Here is a patch.

I can confirm that this patch doesn't seem to cause any negative side 
effects on either pktsetup or pktcdvd, which are the only two user space 
programs I know of that care about the pktcdvd driver.

Acked-by: Peter Osterlund <petero2@telia.com>


> Thanks,
> Kay
>
>
> From: Kay Sievers <kay.sievers@vrfy.org>
> Subject: pktcdvd: remove broken dev_t export of class devices
>
> The pktcdvd created class devices only export some sysfs files,
> but have no char dev_t registered in the driver.
>
> At class device creation time they copy the dev_t value of the
> block device to the char device, wich will register a new char
> device in the driver core and userspace, with a conflicting dev_t
> value.
>
> In many cases the class devices dev_t just points to a random
> USB device. This fixes the sysfs "duplicate entry" errors.
>
> Cc: petero2@telia.com
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> ---
> pktcdvd.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/block/pktcdvd.c
> +++ b/drivers/block/pktcdvd.c
> @@ -302,7 +302,7 @@ static struct kobj_type kobj_pkt_type_wqueue = {
> static void pkt_sysfs_dev_new(struct pktcdvd_device *pd)
> {
> 	if (class_pktcdvd) {
> -		pd->dev = device_create(class_pktcdvd, NULL, pd->pkt_dev, NULL,
> +		pd->dev = device_create(class_pktcdvd, NULL, MKDEV(0, 0), NULL,
> 					"%s", pd->name);
> 		if (IS_ERR(pd->dev))
> 			pd->dev = NULL;

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [PATCH] fix pktcdvd breakage from commit e105b8bfc769b0545b6f0f395179d1e43cbee822

[Kay Sievers]

On Sun, Nov 30, 2008 at 14:32, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Nov 30, 2008 at 02:25:43PM +0100, Kay Sievers wrote:
>
>> Pktcdvd creates char _and_ block device nodes at the same time, while
>> the char nodes are not allocated, but created and conflict in
>> /sys/dev/ with properly allocated ones from other subsystems. Your
>> patch just papers over this bug.
>
> Where the hell is it creating any char device nodes?  Show me.

Here the hell it is:
  pd->dev = device_create(class_pktcdvd, NULL, pd->pkt_dev, NULL,
"%s", pd->name);

The bogus "pkt_dev", it's a char dev_t value, but because the char
dev_t value is already validly used by other subsystems, it will cause
the broken /sys/dev/ link. That's the real bug.

Kay