[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] block/monitor: blk_bs() return value check
From: |
Kevin Wolf |
Subject: |
Re: [PATCH] block/monitor: blk_bs() return value check |
Date: |
Fri, 24 Nov 2023 17:49:20 +0100 |
Am 24.11.2023 um 15:05 hat Дмитрий Фролов geschrieben:
>
>
> On 24.11.2023 16:06, Kevin Wolf wrote:
> > Am 24.11.2023 um 12:30 hat Dmitry Frolov geschrieben:
> > > blk_bs() may return NULL, which will be dereferenced without a check in
> > > bdrv_commit().
> > >
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > >
> > > Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> > Do you have a reproducer for a crash?
> Actually, there was no crash. This problem was found by static analyzer.
> > As far as I can see, it will not be dereferenced, because
> > blk_is_available() returns false and we return an error before
> > calling bdrv_commit():
> As I see, there are 2 reasons, why blk_bs() may return NULL:
> blk->root == NULL or blk->root->bs == NULL
blk->root->bs == NULL shouldn't happen, but the code we're looking at is
safe even for this case.
> At the same time, blk_is_available() checks for
> blk_co_is_inserted(blk) and blk_dev_is_tray_open(blk).
> Does it also guarantee that blk->root and blk->root->bs are not NULL?
> This is not really obvious.
blk_co_is_inserted() does, it returns false for blk_bs(blk) == NULL.
> Maybe, in this case, it makes sense to check blk->root before of
> checking blk_is_available()?
Checking blk->root and those few other things is a really common thing
that most operations do, which is why we have blk_is_available() to
check all of this. If we did the checks before calling it, we wouldn't
need blk_is_available() any more.
Kevin