| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 15 Nov 2010 11:28:02 +0100
From: Jiri Slaby <jslaby@...e.cz>
To: Baruch Siach <baruch@...s.co.il>
CC: linux-kernel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>,
Indan Zupancic <indan@....nu>, Greg KH <greg@...ah.com>,
"H. Peter Anvin" <hpa@...or.com>,
Alex Gershgorin <agersh@...bler.ru>
Subject: Re: [PATCHv3] drivers/misc: Altera active serial implementation
On 11/15/2010 11:08 AM, Baruch Siach wrote:
> Hi Jiri,
>
> Thanks for reviewing. See my response to your comments below.
>
> On Mon, Nov 15, 2010 at 10:33:37AM +0100, Jiri Slaby wrote:
>> On 11/15/2010 09:23 AM, Baruch Siach wrote:
>>> --- /dev/null
>>> +++ b/drivers/misc/altera_as.c
>>> @@ -0,0 +1,349 @@
>> ...
>>> +struct altera_as_device {
>>> + unsigned id;
>>> + struct device *dev;
>>> + struct miscdevice miscdev;
>>> + struct mutex open_lock;
>>> + struct gpio gpios[AS_GPIO_NUM];
>>> +};
>>> +
>>> +/*
>>> + * The only functions updating this array are .probe/.remove, which are
>>> + * serialized. Hence, no locking is needed here.
>>> + */
>>> +static struct {
>>> + int minor;
>>
>> Why you need minor here? It's in drvdata->miscdev.minor.
>
> I use the minor field to lookup the drvdata pointer in get_as_dev(), which I
> use is altera_as_open(). I do this because I have no access to the 'struct
> device' pointer from the .open method. Do you know a better way?
>
>>> + struct altera_as_device *drvdata;
>>> +} altera_as_devs[AS_MAX_DEVS];
>>
>> You don't need the struct at all.
>> static struct altera_as_device *drvdata[AS_MAX_DEVS];
>> should be enough.
>
> See above.
The answer to you previous question is here. You can just have a global
array of struct altera_as_device.
>>> +static int altera_as_open(struct inode *inode, struct file *file)
>>> +{
>>> + int ret;
>>> + struct altera_as_device *drvdata = get_as_dev(iminor(inode));
>>> +
>>> + if (drvdata == NULL)
>>> + return -ENODEV;
>>> + file->private_data = drvdata;
>>> +
>>> + ret = mutex_trylock(&drvdata->open_lock);
>>> + if (ret == 0)
>>> + return -EBUSY;
>>> +
>>> + ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
>>> + if (ret < 0) {
>>> + mutex_unlock(&drvdata->open_lock);
>>> + return ret;
>>> + }
>>
>> So leaving to userspace with mutex held. This is *wrong*. use
>> test_and_set_bit or alike instead.
>
> Can you explain why this is wrong? I don't mind doing test_and_set_bit
> instead, I'm just curious.
The mutex has an owner which it expects to unlock it. For example if you
fork a process which already opened the device, you have a problem. This
is a technical point. Another point is that it's ugly to leave to
userspace with any lock.
>>> +static int __init altera_as_probe(struct platform_device *pdev)
>>> +{
>> ...
>>> + drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
>>> + drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
>>> + pdata->id);
>>> + if (drvdata->miscdev.name == NULL)
>>> + return -ENOMEM;
>>> + drvdata->miscdev.fops = &altera_as_fops;
>>> + if (misc_register(&drvdata->miscdev) < 0) {
>>
>> Hmm, how many devices can be in the system?
>
> Up to AS_MAX_DEVS, currently 16.
>
>> This doesn't look like the right way.
>
> What is the right way then?
Ok, so for that count it definitely deserves its own major to not eat up
misc device space.
>>> + kfree(drvdata->miscdev.name);
>>> + return -ENODEV;
>>> + }
>>> + altera_as_devs[pdata->id].minor = drvdata->miscdev.minor;
>>> + altera_as_devs[pdata->id].drvdata = drvdata;
>>
>> So now the device is usable without the mutex and gpios inited?
>
> I was thinking that the device lock which is being held during the .probe run,
> prevents the device from being open. After all I can still return -EGOAWAY at
> this point. Am I wrong?
>
> If so, I'll reorder this code.
This cannot be done easily. You need to set drvdata prior to minor and
after all the assignments here. The former because in get_as_dev you
test minor and return drvdata. The latter because you use open_lock &
gpios after playing with minor & drvdata.
Technically, it can be done with a use of barriers, but I don't
recommend it as drivers should not use barriers at all. You should
introduce some locking here (it introduces barriers on its own).
So move the assignment to altera_as_devs below the mutex_init & gpios
setup and lock it appropriately. Then add a lock to get_as_dev.
>>> + mutex_init(&drvdata->open_lock);
>>> +
>>> + drvdata->id = pdata->id;
>>> +
>>> + drvdata->gpios[ASIO_DATA].gpio = pdata->data;
>>> + drvdata->gpios[ASIO_DATA].flags = GPIOF_IN;
>>> + drvdata->gpios[ASIO_DATA].label = "as_data";
>> ...
>>> + drvdata->gpios[ASIO_NCE].gpio = pdata->nce;
>>> + drvdata->gpios[ASIO_NCE].flags = GPIOF_OUT_INIT_HIGH;
>>> + drvdata->gpios[ASIO_NCE].label = "as_nce";
>>> +
>>> + dev_info(drvdata->dev, "Altera Cyclone Active Serial driver\n");
>>> +
>>> + return 0;
>>> +}
regards,
--
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists