Hi Christoph, I understand and agree with your concern. Looking at the code, bi_flags is 16-bits wide, and only top 3 bits are preserved by bio_reset(). These bits are used to indicate a bvec pool. So we don't have any free bits to move the BIO_REFFED to. Unless we make bi_flags wider. Bottom line, now if somebody calls bio_get() and then bio_reset(), then on next bio_put() bio will be freed immediately. But this is wrong, because eventually another bio_put() will be done, and this will result in double-free and kernel memory corruption. For example, bio_map_user_iov() performs an additional bio_get() before returning. If somebody calls bio_reset() in-between, then bio_unmap_user() will do double-free. So in general, I think this issue needs to be taken care of, even if not the way I suggested. Thanks, Alex. On Sun, Apr 7, 2019 at 10:56 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > > On Sat, Apr 06, 2019 at 12:58:54PM -0600, Jens Axboe wrote: > > On 4/6/19 7:03 AM, Alex Lyakas wrote: > > > Commit dac56212e8127dbc0bff7be35c508bc280213309 titled > > > "bio: skip atomic inc/dec of ->bi_cnt for most use cases" > > > made __bi_cnt dependent on the new BIO_REFFED flag. > > > > > > bio_reset() does not reset __bi_cnt, but it does reset the BIO_REFFED flag. > > > But __bi_cnt depends now on the BIO_REFFED flag, so bio_reset() > > > needs to preserve this flag. > > > > Looks good to me, applied, thanks. > > Uh-oh. While this fix isn't wrong per se I think it is confusing and > set a dnagerous precedence. > > We have BIO_RESET_BITS to indicate which flags survive. So any flag > that is supposed to survive the reset should be be added to that > instead of specifically worked around.
