[PATCH] scsi: ses check name in enclosure_component_register

[PATCH] scsi: ses check name in enclosure_component_register

[Yinghai Lu]

dev_set_name will use sprintf to copy the name.
need to check if the name does valid.

otherwise will error from device_add later.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 3cf61ec..68743a9 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -255,7 +255,7 @@ enclosure_component_register(struct enclosure_device *edev,
 	ecomp->number = number;
 	cdev = &ecomp->cdev;
 	cdev->parent = get_device(&edev->edev);
-	if (name)
+	if (name && name[0])
 		dev_set_name(cdev, name);
 	else
 		dev_set_name(cdev, "%u", number);

Re: [PATCH] scsi: ses check name in enclosure_component_register

[James Bottomley]

On Mon, 2009-04-27 at 19:18 -0700, Yinghai Lu wrote:
> dev_set_name will use sprintf to copy the name.
> need to check if the name does valid.
> 
> otherwise will error from device_add later.

I think what you mean to say is that empty names aren't allowed in the
device model.

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index 3cf61ec..68743a9 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -255,7 +255,7 @@ enclosure_component_register(struct enclosure_device *edev,
>  	ecomp->number = number;
>  	cdev = &ecomp->cdev;
>  	cdev->parent = get_device(&edev->edev);
> -	if (name)
> +	if (name && name[0])
>  		dev_set_name(cdev, name);

Actually, I think this should become

dev_set_name(cdev, "%s", name);

as well, otherwise any name with a percent in it will get interpreted in
ways we're not expecting at all.

James

Re: [PATCH] scsi: ses check name in enclosure_component_register

[Yinghai Lu]

James Bottomley wrote:
> On Mon, 2009-04-27 at 19:18 -0700, Yinghai Lu wrote:
>> dev_set_name will use sprintf to copy the name.
>> need to check if the name does valid.
>>
>> otherwise will error from device_add later.
> 
> I think what you mean to say is that empty names aren't allowed in the
> device model.
> 
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
>> index 3cf61ec..68743a9 100644
>> --- a/drivers/misc/enclosure.c
>> +++ b/drivers/misc/enclosure.c
>> @@ -255,7 +255,7 @@ enclosure_component_register(struct enclosure_device *edev,
>>  	ecomp->number = number;
>>  	cdev = &ecomp->cdev;
>>  	cdev->parent = get_device(&edev->edev);
>> -	if (name)
>> +	if (name && name[0])
>>  		dev_set_name(cdev, name);
> 
> Actually, I think this should become
> 
> dev_set_name(cdev, "%s", name);
> 
> as well, otherwise any name with a percent in it will get interpreted in
> ways we're not expecting at all.

i tried that it still failed.

YH

Re: [PATCH] scsi: ses check name in enclosure_component_register

[James Bottomley]

On Tue, 2009-04-28 at 16:56 -0700, Yinghai Lu wrote:
> James Bottomley wrote:
> > On Mon, 2009-04-27 at 19:18 -0700, Yinghai Lu wrote:
> >> dev_set_name will use sprintf to copy the name.
> >> need to check if the name does valid.
> >>
> >> otherwise will error from device_add later.
> > 
> > I think what you mean to say is that empty names aren't allowed in the
> > device model.
> > 
> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >>
> >> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> >> index 3cf61ec..68743a9 100644
> >> --- a/drivers/misc/enclosure.c
> >> +++ b/drivers/misc/enclosure.c
> >> @@ -255,7 +255,7 @@ enclosure_component_register(struct enclosure_device *edev,
> >>  	ecomp->number = number;
> >>  	cdev = &ecomp->cdev;
> >>  	cdev->parent = get_device(&edev->edev);
> >> -	if (name)
> >> +	if (name && name[0])
> >>  		dev_set_name(cdev, name);
> > 
> > Actually, I think this should become
> > 
> > dev_set_name(cdev, "%s", name);
> > 
> > as well, otherwise any name with a percent in it will get interpreted in
> > ways we're not expecting at all.
> 
> i tried that it still failed.

I didn't say it was the fix to your problem.  I said on looking at the
code it's another potential problem source.  Your problem is that
filesystems don't support empty file names.

James

Re: [PATCH] scsi: ses check name in enclosure_component_register

[Yinghai Lu]

James Bottomley wrote:
> On Tue, 2009-04-28 at 16:56 -0700, Yinghai Lu wrote:
>> James Bottomley wrote:
>>> On Mon, 2009-04-27 at 19:18 -0700, Yinghai Lu wrote:
>>>> dev_set_name will use sprintf to copy the name.
>>>> need to check if the name does valid.
>>>>
>>>> otherwise will error from device_add later.
>>> I think what you mean to say is that empty names aren't allowed in the
>>> device model.
>>>
>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>>
>>>> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
>>>> index 3cf61ec..68743a9 100644
>>>> --- a/drivers/misc/enclosure.c
>>>> +++ b/drivers/misc/enclosure.c
>>>> @@ -255,7 +255,7 @@ enclosure_component_register(struct enclosure_device *edev,
>>>>  	ecomp->number = number;
>>>>  	cdev = &ecomp->cdev;
>>>>  	cdev->parent = get_device(&edev->edev);
>>>> -	if (name)
>>>> +	if (name && name[0])
>>>>  		dev_set_name(cdev, name);
>>> Actually, I think this should become
>>>
>>> dev_set_name(cdev, "%s", name);
>>>
>>> as well, otherwise any name with a percent in it will get interpreted in
>>> ways we're not expecting at all.
>> i tried that it still failed.
> 
> I didn't say it was the fix to your problem.  I said on looking at the
> code it's another potential problem source.  Your problem is that
> filesystems don't support empty file names.

dev_set_name(cdev, name);

the name become fmt. 

YH

Re: [PATCH] scsi: ses check name in enclosure_component_register

[Matthew Wilcox]

On Tue, Apr 28, 2009 at 06:02:23PM -0700, Yinghai Lu wrote:
> dev_set_name(cdev, name);
> 
> the name become fmt. 

Yes, and what happens if 'name' is '%s'?

That's James's point.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] scsi: ses check name in enclosure_component_register

[Yinghai Lu]

Matthew Wilcox wrote:
> On Tue, Apr 28, 2009 at 06:02:23PM -0700, Yinghai Lu wrote:
>> dev_set_name(cdev, name);
>>
>> the name become fmt. 
> 
> Yes, and what happens if 'name' is '%s'?
> 

change
dev_set_name(cdev, name);
to
dev_set_name(cdev, "%s", name);

didn't fix the problem.

need to check name && name[0]

YH

Re: [PATCH] scsi: ses check name in enclosure_component_register

[Matthew Wilcox]

On Tue, Apr 28, 2009 at 08:33:46PM -0700, Yinghai Lu wrote:
> Matthew Wilcox wrote:
> > On Tue, Apr 28, 2009 at 06:02:23PM -0700, Yinghai Lu wrote:
> >> dev_set_name(cdev, name);
> >>
> >> the name become fmt. 
> > 
> > Yes, and what happens if 'name' is '%s'?
> > 
> 
> change
> dev_set_name(cdev, name);
> to
> dev_set_name(cdev, "%s", name);
> 
> didn't fix the problem.
> 
> need to check name && name[0]

Yes.  There are _two_ problems.  You fixed one.  James asked if you
would fix the other while you're touching that code.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

[PATCH] scsi: ses check name in enclosure_component_register -v2

[Yinghai Lu]

dev_set_name will use sprintf to copy the name.
need to check if the name does valid.

otherwise will error from device_add later.

v2: add %s in dev_set_name according to James

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/misc/enclosure.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/misc/enclosure.c
===================================================================
--- linux-2.6.orig/drivers/misc/enclosure.c
+++ linux-2.6/drivers/misc/enclosure.c
@@ -119,7 +119,7 @@ enclosure_register(struct device *dev, c
 	edev->edev.class = &enclosure_class;
 	edev->edev.parent = get_device(dev);
 	edev->cb = cb;
-	dev_set_name(&edev->edev, name);
+	dev_set_name(&edev->edev, "%s", name);
 	err = device_register(&edev->edev);
 	if (err)
 		goto err;
@@ -255,8 +255,8 @@ enclosure_component_register(struct encl
 	ecomp->number = number;
 	cdev = &ecomp->cdev;
 	cdev->parent = get_device(&edev->edev);
-	if (name)
-		dev_set_name(cdev, name);
+	if (name && name[0])
+		dev_set_name(cdev, "%s", name);
 	else
 		dev_set_name(cdev, "%u", number);