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 <[email protected]> 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 <[email protected]> wrote: >> >> > From: Joonwoo Park <[email protected]> >> > Subject: Re: [Click] click Digest, Vol 92, Issue 4 >> > To: "Bobby Longpocket" <[email protected]> >> > Cc: [email protected] >> > Date: Wednesday, February 2, 2011, 11:20 AM >> > Bobby, >> > >> > On Wed, Feb 2, 2011 at 11:04 AM, Joonwoo Park <[email protected]> >> > 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 >> > >> >> >> > _______________________________________________ click mailing list [email protected] https://amsterdam.lcs.mit.edu/mailman/listinfo/click
