Locking IOCTL

Locking IOCTL

[Praveen kumar]

 Hi All,
I have a situation where I have to lock the ioctl provided in my driver. I
have a uni processor (ARM) system.
I am using Mutex as the lock for my ioctl.
DEFINE_MUTEX(&lock_ioctl);
MyIoctl()
{
Mutex_lock(&lock_ioctl);
Switch(){
...........
}
Mutex_unlock(&lock_ioctl);
return 0;
}
I just wanted to know am I using the best lock available for the situation
aor do I have any flaw in my implementation???

And from LKD I  read that "*lock the data not the code*" and which I am
literally doing so ?? any comments on this ?

I have interrupts in my driver which is nothing to do with the lock.I am
taking care that where ever I return in my ioctl the lock is released.

Thanks ,
Praveen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20111115/cfaeae49/attachment.html 

Locking IOCTL

[Dave Hylands]

Hi Praveen,

On Mon, Nov 14, 2011 at 11:22 PM, Praveen kumar <chatpravi@gmail.com> wrote:
>
> Hi All,
> I have a situation where I have to lock the ioctl provided in my driver. I
> have a uni processor (ARM) system.
> I am using Mutex as the lock for my ioctl.
> DEFINE_MUTEX(&lock_ioctl);
> MyIoctl()
> {
> Mutex_lock(&lock_ioctl);
> Switch(){
> ...........
> }
> Mutex_unlock(&lock_ioctl);
> return 0;
> }
> I just wanted to know am I using the best lock available for the situation
> aor do I have any flaw in my implementation???
>
> And from LKD I ?read that "*lock the data not the code*" and which I am
> literally doing so ?? any comments on this ?

By creatiing something called lock_ioctl, I'd say you're locking the code.

Locking the data means that lets suppose you have some data structure
which requires mutual exclusion. Then I'd create a lock associated
with that data structure (and probably the lock would be a member of
the data structure), and anytime I needed to manipulate/access the
data structure (in an atomic manner), I'd acquire the lock.

> I have interrupts in my driver which is nothing to do with the lock.I am
> taking care that where ever I return in my ioctl the lock is released.

What are you locking? Does every single ioctl really need to acquire
the lock? Why?

-- 
Dave Hylands
Shuswap, BC, Canada
http://www.davehylands.com

Locking IOCTL

[Praveen kumar]

The driver is opened by multiple processes, One such process is a
diagnostics.
Where I am checking sanity of some of the registers if I make ioctl open
for all the processes.It can screw
up my diagnostics check.(write some values to registers). This is basic
reason I am using lock to my ioctl.

Thanks
Praveen

On Tue, Nov 15, 2011 at 1:28 PM, Dave Hylands <dhylands@gmail.com> wrote:

> Hi Praveen,
>
> On Mon, Nov 14, 2011 at 11:22 PM, Praveen kumar <chatpravi@gmail.com>
> wrote:
> >
> > Hi All,
> > I have a situation where I have to lock the ioctl provided in my driver.
> I
> > have a uni processor (ARM) system.
> > I am using Mutex as the lock for my ioctl.
> > DEFINE_MUTEX(&lock_ioctl);
> > MyIoctl()
> > {
> > Mutex_lock(&lock_ioctl);
> > Switch(){
> > ...........
> > }
> > Mutex_unlock(&lock_ioctl);
> > return 0;
> > }
> > I just wanted to know am I using the best lock available for the
> situation
> > aor do I have any flaw in my implementation???
> >
> > And from LKD I  read that "*lock the data not the code*" and which I am
> > literally doing so ?? any comments on this ?
>
> By creatiing something called lock_ioctl, I'd say you're locking the code.
>
> Locking the data means that lets suppose you have some data structure
> which requires mutual exclusion. Then I'd create a lock associated
> with that data structure (and probably the lock would be a member of
> the data structure), and anytime I needed to manipulate/access the
> data structure (in an atomic manner), I'd acquire the lock.
>
> > I have interrupts in my driver which is nothing to do with the lock.I am
> > taking care that where ever I return in my ioctl the lock is released.
>
> What are you locking? Does every single ioctl really need to acquire
> the lock? Why?
>
> --
> Dave Hylands
> Shuswap, BC, Canada
> http://www.davehylands.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20111115/27572f5c/attachment-0001.html 

Locking IOCTL

[Tirtha Ghosh]

Which kernel version are u using???

As far as I know, before 2.6.36, ioctl calls used to hold BKL, so no need
to have separate locking mechanism.

If you are using unlocked_ioctl (after 36 kernel) then you need to take
care of locking. Now, what kind of lock you want to use, depends on your
requirement. It could be code lock or data lock..




On Tue, Nov 15, 2011 at 5:53 PM, Praveen kumar <chatpravi@gmail.com> wrote:

> The driver is opened by multiple processes, One such process is a
> diagnostics.
> Where I am checking sanity of some of the registers if I make ioctl open
> for all the processes.It can screw
> up my diagnostics check.(write some values to registers). This is basic
> reason I am using lock to my ioctl.
>
> Thanks
> Praveen
>
>
> On Tue, Nov 15, 2011 at 1:28 PM, Dave Hylands <dhylands@gmail.com> wrote:
>
>> Hi Praveen,
>>
>> On Mon, Nov 14, 2011 at 11:22 PM, Praveen kumar <chatpravi@gmail.com>
>> wrote:
>> >
>> > Hi All,
>> > I have a situation where I have to lock the ioctl provided in my
>> driver. I
>> > have a uni processor (ARM) system.
>> > I am using Mutex as the lock for my ioctl.
>> > DEFINE_MUTEX(&lock_ioctl);
>> > MyIoctl()
>> > {
>> > Mutex_lock(&lock_ioctl);
>> > Switch(){
>> > ...........
>> > }
>> > Mutex_unlock(&lock_ioctl);
>> > return 0;
>> > }
>> > I just wanted to know am I using the best lock available for the
>> situation
>> > aor do I have any flaw in my implementation???
>> >
>> > And from LKD I  read that "*lock the data not the code*" and which I am
>> > literally doing so ?? any comments on this ?
>>
>> By creatiing something called lock_ioctl, I'd say you're locking the code.
>>
>> Locking the data means that lets suppose you have some data structure
>> which requires mutual exclusion. Then I'd create a lock associated
>> with that data structure (and probably the lock would be a member of
>> the data structure), and anytime I needed to manipulate/access the
>> data structure (in an atomic manner), I'd acquire the lock.
>>
>> > I have interrupts in my driver which is nothing to do with the lock.I am
>> > taking care that where ever I return in my ioctl the lock is released.
>>
>> What are you locking? Does every single ioctl really need to acquire
>> the lock? Why?
>>
>> --
>> Dave Hylands
>> Shuswap, BC, Canada
>> http://www.davehylands.com
>>
>
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20111115/24c493c5/attachment.html 

Scull -unable to handle kernel paging request

[anish kumar]

Hello,

Testing the scullwuid produced attached crash logs.

Combination of read and write using "dd" and "cat"
caused this problem.

I don't exactly remember exactly what caused the problem
but combination of read and write did it.

Reproduction would not be possible but just want to know
why this happened as anyone looking into the code would
come to the conclusion that proper locking is in place.

Below is small part of the crash logs:

[21028.412827] BUG: unable to handle kernel paging request at 00300d04
[21028.412832] IP: [<c035aef7>] _copy_from_user+0x97/0x130
[21028.412839] *pde = 6fa0b067 
[21028.412842] Oops: 0003 [#1] SMP 

[21028.412933] Call Trace:
[21028.412940]  [<f9f24d84>] ? scull_write+0x193/0x204 [scull]
[21028.412946]  [<c0218fc2>] ? vfs_write+0xa2/0x190
[21028.412949]  [<f9f24bf1>] ? scull_write+0x0/0x204 [scull]
[21028.412953]  [<c0219882>] ? sys_write+0x42/0x70
[21028.412958]  [<c05cadd4>] ? syscall_call+0x7/0xb
[21028.412963]  [<c05c0000>] ? calibrate_delay_direct+0x5a/0xfb

Some of the below code is removed for fitting it into this mail:
ssize_t scull_write(struct file *filp, const char __user *buf, size_t count,
                loff_t *f_pos)
{
        if (down_interruptible(&dev->sem))
                return -ERESTARTSYS;
        dptr = scull_follow(dev, item);
        if (dptr == NULL)
                goto out;
        if (!dptr->data) {
                dptr->data = kmalloc(qset * sizeof(char *), GFP_KERNEL);
                if (!dptr->data)
                        goto out;
                memset(dptr->data, 0, qset * sizeof(char *));
        }
        if (!dptr->data[s_pos]) {
                dptr->data[s_pos] = kmalloc(quantum, GFP_KERNEL);
                if (!dptr->data[s_pos])
                        goto out;
        /* write only up to the end of this quantum */
        if (count > quantum - q_pos) 
                count = quantum - q_pos;

        if (copy_from_user(dptr->data[s_pos]+q_pos, buf, count)) {
                retval = -EFAULT;
                goto out;
        }


Used: https://github.com/martinezjavier/ldd3 
box:Ubuntu 10.10 |2.6.35-27-generic #48-Ubuntu SMP i686 GNU/Linux

--thanks


-------------- next part --------------
[21028.412827] BUG: unable to handle kernel paging request at 00300d04
[21028.412832] IP: [<c035aef7>] _copy_from_user+0x97/0x130
[21028.412839] *pde = 6fa0b067 
[21028.412842] Oops: 0003 [#1] SMP 
[21028.412845] last sysfs file: /sys/devices/pci0000:00/0000:00:1c.3/0000:09:00.0/local_cpus
[21028.412849] Modules linked in: scull rfcomm binfmt_misc sco bnep l2cap parport_pc ppdev dm_crypt snd_hda_codec_hdmi snd_hda_codec_idt snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq lib80211_crypt_tkip btusb snd_timer snd_seq_device uvcvideo bluetooth videodev dell_wmi wl(P) v4l1_compat psmouse serio_raw dell_wmi_aio sparse_keymap dell_laptop dcdbas snd lib80211 soundcore snd_page_alloc lp parport dm_raid45 xor i915 drm_kms_helper drm usb_storage intel_agp ahci agpgart i2c_algo_bit video output libahci r8169 mii
[21028.412890] 
[21028.412894] Pid: 8241, comm: dd Tainted: P            2.6.35-27-generic #48-Ubuntu 01HXXJ/Inspiron N5050
[21028.412897] EIP: 0060:[<c035aef7>] EFLAGS: 00010216 CPU: 2
[21028.412900] EIP is at _copy_from_user+0x97/0x130
[21028.412902] EAX: 9f034959 EBX: 00000200 ECX: 00000200 EDX: 477efb2f
[21028.412905] ESI: 08e01000 EDI: 00300d04 EBP: c8b6bf1c ESP: c8b6bf10
[21028.412907]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
[21028.412910] Process dd (pid: 8241, ti=c8b6a000 task=f71d2610 task.ti=c8b6a000)
[21028.412912] Stack:
[21028.412913]  f9f26d00 00000cc0 f9f26d14 c8b6bf64 f9f24d84 f9f2636f 0000022d 000002e0
[21028.412920] <0> 00000200 08b74600 00000024 000002e0 000008b4 f247c2d8 08e01000 00000200
[21028.412926] <0> 003d0900 00000200 e677cc80 00000200 08e01000 c8b6bf8c c0218fc2 c8b6bf98
[21028.412933] Call Trace:
[21028.412940]  [<f9f24d84>] ? scull_write+0x193/0x204 [scull]
[21028.412946]  [<c0218fc2>] ? vfs_write+0xa2/0x190
[21028.412949]  [<f9f24bf1>] ? scull_write+0x0/0x204 [scull]
[21028.412953]  [<c0219882>] ? sys_write+0x42/0x70
[21028.412958]  [<c05cadd4>] ? syscall_call+0x7/0xb
[21028.412963]  [<c05c0000>] ? calibrate_delay_direct+0x5a/0xfb
[21028.412965] Code: 8b 1c 24 8b 7c 24 08 89 ec 5d c3 90 89 f0 31 f8 85 05 80 8e 81 c0 74 be 89 d9 8b 46 20 83 f9 43 76 04 8b 46 40 90 8b 06 8b 56 04 <89> 07 89 57 04 8b 46 08 8b 56 0c 89 47 08 89 57 0c 8b 46 10 8b 
[21028.413001] EIP: [<c035aef7>] _copy_from_user+0x97/0x130 SS:ESP 0068:c8b6bf10
[21028.413005] CR2: 0000000000300d04
[21028.413009] ---[ end trace 3bd8365f04ad063d ]---

Scull -unable to handle kernel paging request

[Javier Martinez Canillas]

On Sat, Nov 19, 2011 at 1:34 PM, anish kumar
<anish198519851985@gmail.com> wrote:
> Hello,
>
> Testing the scullwuid produced attached crash logs.
>

Hello Anish,

Sadly I don't have more time to keep updates the LDD3 examples. I did
a long time ago to use for academic purposes. As you can see my last
commit is from almost a year ago and even then I only used to be sure
that the modules compiles cleanly without warning (not sure if they
compile anymore, probably not) but didn't try it, since I don't use
the modules anymore.

Hope that someone with more time than me can fork the git tree and
continue with this work since the LDD3 book is an amazing book to
introduce newcomers to Linux driver development.

Best regards,

-- 
Javier Mart?nez Canillas
(+34) 682 39 81 69
Barcelona, Spain

Locking IOCTL

[Dave Hylands]

Hi Praveen,

On Tue, Nov 15, 2011 at 4:23 AM, Praveen kumar <chatpravi@gmail.com> wrote:
> The driver is opened by multiple processes, One such process is a
> diagnostics.
> Where I am checking sanity of some of the registers if I make ioctl open for
> all the processes.It can screw
> up my diagnostics check.(write some values to registers). This is basic
> reason I am using lock to my ioctl.

So "locking the data" means that you should create a lock for the data
registers in question.

You should acquire the lock around writing the values to the
registers, and presumably around reading them.

Acquiring the lock is about accessing the registers, not calling the
ioctl, even if today the only place that you access the registers is
from the ioctl. What happens if down the road you decide to add some
type of monitoring thread and it needs to access the registers? It's
not in an ioctl, so acquiring some type of ioctl lock is misleading.
The data that the lock is tied to is the registers, so make the code
work that way.

Also, som paths through the ioctl might not need to access the
registers, so you don't need or want to acquire the lock for those
cases.

-- 
Dave Hylands
Shuswap, BC, Canada
http://www.davehylands.com