[Click] ToUserDevice IOCTL

[Joonwoo Park]

Actually, I think even reading MSBs is not fine either if
filp->private_data is shared.
The thing is assigning private_data is not atomic operation so don't
know what can temporarily exist while assignment  is going even though
MSBs are not changing.  It's totally up to how opcode works to
specific platform.
So in that chase I think the other thread can read invalid MSBs.

Regards,
Joonwoo

On Wed, Feb 2, 2011 at 1:53 PM, Joonwoo Park <joonwpark81 at gmail.com> wrote:
> Hi Bobby,
>
> Hmm.. I think it depends on the terminology of 'valid'.
> I just checked code.  I think flip->private_data's stored value is valid only to the GETELEM macro's perspective.
> If multiple threads race with shared filp->private_data, without having critical section, we cannot prevent situation like below.
> thr 1) enter to  set private_data's LSB to 0
> thr 2) enter and get private_data's LSB and exit. (it's not 0 yet)
> thr 1) set private_data's LSB to 0 finally and exit
>
> Now although thr1 set LSB to 0 earlier than thr2, thr2 read non-0 LSB.  I doubt that thr2 read *valid* data at this point.
> Moreover this behaviour wouldn't happen prior to this patch under the BKL.
>
> BUT, at anyway.  I confirmed that filp->private_data is not pointing shared data.  So I'll remove unnecessary mutex.
>
> Thanks!
> Joonwoo
>
> On Wed, Feb 02, 2011 at 01:03:39PM -0800, Bobby Longpocket wrote:
>> Hi Joonwoo,
>>
>> I think it doesn't matter whether the filp is shared by multiple threads or not. The mutex is only protecting the assignment, and the assignment can differ only in the least-significant bit.  No matter how many threads call at once, the stored value will be valid.  There's a race condition as to which thread wins, but that's true even with the mutex.
>>
>> Regards,
>> BBL
>>
>>
>> --- On Wed, 2/2/11, Joonwoo Park <joonwpark81 at gmail.com> wrote:
>>
>> > From: Joonwoo Park <joonwpark81 at gmail.com>
>> > Subject: Re: [Click] click Digest, Vol 92, Issue 4
>> > To: "Bobby Longpocket" <bobbylongpocket at yahoo.com>
>> > Cc: click at pdos.csail.mit.edu
>> > Date: Wednesday, February 2, 2011, 11:20 AM
>> > Bobby,
>> >
>> > On Wed, Feb 2, 2011 at 11:04 AM, Joonwoo Park <joonwpark81 at gmail.com>
>> > wrote:
>> > >> In ToUserDevice:  It looks like the existing
>> > IOCTL isn't doing anything that requires locking, so you
>> > should be able to leave out the mutex.
>> > >
>> > > I don't think so...  filp->private_data is not
>> > atomic on every platform.
>> > >
>> >
>> > I have to double check tonight (I'm not accessing click
>> > source code at
>> > work) but I think it's possible I was wrong.
>> > I was thinking that filp->private_data is shared between
>> > different
>> > userspace open.  But it's probably not true.
>> > I'll have look again.
>> >
>> > Thanks!
>> > Joonwoo
>> >
>>
>>
>>
>