On Sat, Nov 13, 2010 at 1:28 AM, Joe Eykholt <jeykholt@xxxxxxxxx> wrote:
>
>
> On 11/12/10 5:42 AM, Hillf Danton wrote:
>> On Fri, Nov 12, 2010 at 2:47 AM, Joe Eykholt <jeykholt@xxxxxxxxx> wrote:
>>>
>>>
>>> On 11/11/10 5:45 AM, Hillf Danton wrote:
>>>> On Wed, Nov 10, 2010 at 2:15 AM, Joe Eykholt <jeykholt@xxxxxxxxx> wrote:
>>>>> On 11/9/10 6:01 AM, Hillf Danton wrote:
>>>>>> Ring buffers are setup for exchanging data between K and U spaces, but
>>>>>> they could not survive multiple open operations.
>>>>>>
>>>>>> The registered misc interface is monitored and prevented from multiple
>>>>>> opens for fixing the vulnerability.
>>>>>>
>>>>>> A typo, -BUSY, is also cleaned up.
>>>>>>
>>>>>> btw, the ring buffers could be setup in a per file manner?
>>>>>>
>>>>>> Signed-off-by: Hillf Danton <dhillf@xxxxxxxxx>
>>>>>> ---
>>>>>>
>>>>>> --- a/drivers/scsi/scsi_tgt_if.c   Â2010-09-13 07:07:38.000000000 +0800
>>>>>> +++ b/drivers/scsi/scsi_tgt_if.c   Â2010-11-09 21:42:48.000000000 +0800
>>>>>> @@ -85,7 +85,7 @@ static int tgt_uspace_send_event(u32 typ
>>>>>> Â Â Â if (!ev->hdr.status)
>>>>>> Â Â Â Â Â Â Â tgt_ring_idx_inc(ring);
>>>>>> Â Â Â else
>>>>>> - Â Â Â Â Â Â err = -BUSY;
>>>>>> + Â Â Â Â Â Â err = -EBUSY;
>>>>>>
>>>>>> Â Â Â spin_unlock_irqrestore(&ring->tr_lock, flags);
>>>>>>
>>>>>> @@ -319,20 +319,33 @@ static int tgt_mmap(struct file *filp, s
>>>>>> Â Â Â return err;
>>>>>> Â}
>>>>>>
>>>>>> +static unsigned long tgt_open_cnt = 0;
>>>>>> +
>>>>>> Âstatic int tgt_open(struct inode *inode, struct file *file)
>>>>>> Â{
>>>>>> + Â Â if (tgt_open_cnt)
>>>>>> + Â Â Â Â Â Â return -EBUSY;
>>>>>> + Â Â tgt_open_cnt++;
>>>>>
>>>>> Since there's no locking, there's still a tiny hole where
>>>>> simultaneous opens could succeed. ÂConsider using an atomic.
>>>>> Good find and good fix otherwise.
>>>>>
>>>> Would you please, Joe, show the atomic version?
>>>> thanks//Hillf
>>>
>>> I take it back. ÂThere's no good atomic version.
>>> The best I came up with was:
>>> In open:
>>> Â Â Â Âif (atomic_inc_return(&tgt_open_cnt) != 1)
>>> Â Â Â Â Â Â Â Âreturn -EBUSY;
>>>
>>> Then in release (since its the last close):
>>> Â Â Â Âatomic_set(&tgt_open_cnt, 0);
>>>
>>> There's still a hole that this might overflow, and I don't see
>>> the best way to fix that without test-and-set or compare-and-swap.
>>> We can't just decrement it since the last close will clear it.
>>
>> Great operation, thanks.
>> A good lesson already offered by Wilcox, you see Joe, clearing only
>> necessary when the final closing.
>
> Yes, I think I took that into account.
>
>>> So the best thing would be to use your
>>> version but protect it with the tx_ring.tr_lock.
>>> I would rename tgt_open_cnt to just tgt_busy,
>>> and make it a u8 since it will be 1 or 0.
>>
>> But u8 is not native word, and the spin_lock_irq is enough, I think. //Hillf
>
> I don't understand what you mean by native word. Âu8 is an unsigned char which
I mean unsigned long is more friendly to the 32/64-bit registers of
hardware of this century. And thank you for sharing so much.
If you do not mind, I will prepare new patch based upon busy flag and
spin_lock_irq, and you review it again.
good weekend
Hillf
> is just as natural a data type as an int,
>
> Setting, clearing, and testing a char is just as efficient as an int
> on almost all architectures I can think of, although an loading an
> unsigned char requires masking on one architecture at least,
> so unsigned char can be worse than signed char, but not for comparison
> to zero and storing, which is what we're talking about here.
> That said, it's not a big deal either way. ÂIt's only 3 bytes, and
> this isn't a commonly-used module. ÂSilly of me, really.
>
> But, you still need the busy flag even with the lock.
>
> Â Â Â ÂCheers,
> Â Â Â ÂJoe
>
>>> Â Â Â Âint error = 0;
>>>
>>> Â Â Â Âspin_lock_irq(&tx_ring.tr_lock);
>>> Â Â Â Âif (tgt_busy)
>>> Â Â Â Â Â Â Â Âerror = -EBUSY;
>>> Â Â Â Âelse {
>>> Â Â Â Â Â Â Â Âtgt_busy = 1;
>>> Â Â Â Â Â Â Â Âtx_ring.tr_idx = 0;
>>> Â Â Â Â Â Â Â Ârx_ring.tr_idx = 0;
>>> Â Â Â Â}
>>> Â Â Â Âspin_unlock_irq(&tx_ring.tr_lock);
>>> Â Â Â Âreturn error;
>>>
>>> Then in release:
>>> Â Â Â Âspin_lock_irq(&tx_ring.tr_lock);
>>> Â Â Â Âtgt_busy = 0;
>>> Â Â Â Âspin_unlock_irq(&tx_ring.tr_lock);
>>>
>>>>>> +
>>>>>> Â Â Â tx_ring.tr_idx = rx_ring.tr_idx = 0;
>>>>>>
>>>>>> Â Â Â cycle_kernel_lock();
>>>>>> Â Â Â return 0;
>>>>>> Â}
>>>>>>
>>>>>> +static int tgt_release(struct inode *inode, struct file *file)
>>>>>> +{
>>>>>> + Â Â tgt_open_cnt--;
>>>>>> + Â Â return 0;
>>>>>> +}
>>>>>> +
>>>>>> Âstatic const struct file_operations tgt_fops = {
>>>>>>    .owner     Â= THIS_MODULE,
>>>>>>    .open      = tgt_open,
>>>>>>    .poll      = tgt_poll,
>>>>>>    .write     Â= tgt_write,
>>>>>>    .mmap      = tgt_mmap,
>>>>>> +   .release    Â= tgt_release,
>>>>>> Â};
>>>>>>
>>>>>> Âstatic struct miscdevice tgt_miscdev = {
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>>>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>>>> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
>>>>>
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
