Re: [PATCH 0/5] block subsystem refcounter conversions

[Jens Axboe <axboe@kernel.dk>]

On 02/20/2017 08:41 AM, James Bottomley wrote:
> On Mon, 2017-02-20 at 08:15 -0700, Jens Axboe wrote:
>> On 02/20/2017 04:16 AM, Elena Reshetova wrote:
>>> Now when new refcount_t type and API are finally merged
>>> (see include/linux/refcount.h), the following
>>> patches convert various refcounters in the block susystem from 
>>> atomic_t to refcount_t. By doing this we prevent intentional or 
>>> accidental underflows or overflows that can led to use-after-free
>>> vulnerabilities.
> 
> This description isn't right ... nothing is prevented; we get warnings
> on saturation and use after free with this.
> 
>>> The below patches are fully independent and can be cherry-picked 
>>> separately. Since we convert all kernel subsystems in the same 
>>> fashion, resulting in about 300 patches, we have to group them for 
>>> sending at least in some fashion to be manageable. Please excuse
>>> the long cc list.
>>>
>>> Elena Reshetova (5):
>>>   block: convert bio.__bi_cnt from atomic_t to refcount_t
>>>   block: convert blk_queue_tag.refcnt from atomic_t to refcount_t
>>>   block: convert blkcg_gq.refcnt from atomic_t to refcount_t
>>>   block: convert io_context.active_ref from atomic_t to refcount_t
>>>   block: convert bsg_device.ref_count from atomic_t to refcount_t
>>
>> I went to look at the implementation, and the size of a refcount_t.
>> But the code is not there?! You say it's finally merged, where is
>> it merged? Don't send code like this without the necessary
>> infrastructure being in mainline.
> 
> It's one of those no discussion except notice by tipbot things because
> Ingo liked it.
> 
> The size is atomic_t, but the primitives check for overflow and check
> inc from zero and things, so in a true refcounting situation we get
> automatic warnings of problems inside the primitives.
> 
> That said, if we were going to convert the block layer to this
> semantic, surely the benefit of the conversion would be getting rid of
> the now unnecessary BUG_ON and WARN_ON in the code, which do exactly
> the same thing the refcount primitives now do?

Well, I have no idea what it does, which is why I went to look at the
code. So any talk of converting the block layer is premature.  But it's
not there. I'll defer judgment until we have something in mainline,
until then I've archived this thread.

And I agree, keeping warn/bug for cases that should be handled
with this framework would be counter productive and pointless.

-- 
Jens Axboe