Changwei Ge
2018-Mar-30 02:17 UTC
[Ocfs2-devel] [PATCH] ocfs2: don't evaluate buffer head to NULL managed by caller
Hi Joseph, On 2018/3/30 10:04, Joseph Qi wrote:> > > On 18/3/30 09:31, Changwei Ge wrote: >> Hi Joseph, >> >> On 2018/3/30 9:27, Joseph Qi wrote: >>> >>> >>> On 18/3/29 10:06, Changwei Ge wrote: >>>> ocfs2_read_blocks() is used to read several blocks from disk. >>>> Currently, the input argument *bhs* can be NULL or NOT. It depends on >>>> the caller's behavior. If the function fails in reading blocks from >>>> disk, the corresponding bh will be assigned to NULL and put. >>>> >>>> Obviously, above process for non-NULL input bh is not appropriate. >>>> Because the caller doesn't even know its bhs are put and re-assigned. >>>> >>>> If buffer head is managed by caller, ocfs2_read_blocks should not >>>> evaluate it to NULL. It will cause caller accessing illegal memory, >>>> thus crash. >>>> >>>> Signed-off-by: Changwei Ge <ge.changwei at h3c.com> >>>> --- >>>> fs/ocfs2/buffer_head_io.c | 31 +++++++++++++++++++++++++------ >>>> 1 file changed, 25 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c >>>> index d9ebe11..17329b6 100644 >>>> --- a/fs/ocfs2/buffer_head_io.c >>>> +++ b/fs/ocfs2/buffer_head_io.c >>>> @@ -188,6 +188,7 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, >>>> int i, ignore_cache = 0; >>>> struct buffer_head *bh; >>>> struct super_block *sb = ocfs2_metadata_cache_get_super(ci); >>>> + int new_bh = 0; >>>> >>>> trace_ocfs2_read_blocks_begin(ci, (unsigned long long)block, nr, flags); >>>> >>>> @@ -213,6 +214,18 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, >>>> goto bail; >>>> } >>>> >>>> + /* Use below trick to check if all bhs are NULL or assigned. >>>> + * Basically, we hope all bhs are consistent so that we can >>>> + * handle exception easily. >>>> + */ >>>> + new_bh = (bhs[0] == NULL); >>>> + for (i = 1 ; i < nr ; i++) { >>>> + if ((new_bh && bhs[i]) || (!new_bh && !bhs[i])) { >>>> + WARN(1, "Not all bhs are consistent\n"); >>>> + break; >>>> + } >>>> + } >>>> + >>>> ocfs2_metadata_cache_io_lock(ci); >>>> for (i = 0 ; i < nr ; i++) { >>>> if (bhs[i] == NULL) { >>>> @@ -324,8 +337,10 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, >>>> if (!(flags & OCFS2_BH_READAHEAD)) { >>>> if (status) { >>>> /* Clear the rest of the buffers on error */ >>>> - put_bh(bh); >>>> - bhs[i] = NULL; >>>> + if (new_bh) { >>>> + put_bh(bh); >>>> + bhs[i] = NULL; >>>> + } >>> >>> Since we assume caller has to pass either all NULL or all non-NULL, >>> here we will only put bh internal allocated. Am I missing something? >> >> Thanks for your review. >> Yes, we will only put bh internally allocated. >> If bh is reserved in advance, we will not put it and re-assign it to NULL. >> > > So this branch won't have risk, right?Sorry... I'm not sure if I understand you correctly. This branch will be walked through when previous part of bhs[] faces a read failure in order to put bh allocated in ocfs2_read_blocks(). And we assume all bh should be NULL or non-NULL, if new_bh is set, the back part should also be put to release those buffer heads. If I made a mistake or misunderstand you, please let me know. Thanks, Changwei> > Thanks, > Joseph >
Joseph Qi
2018-Mar-30 02:37 UTC
[Ocfs2-devel] [PATCH] ocfs2: don't evaluate buffer head to NULL managed by caller
On 18/3/30 10:17, Changwei Ge wrote:>>>> Since we assume caller has to pass either all NULL or all non-NULL, >>>> here we will only put bh internal allocated. Am I missing something? >>> Thanks for your review. >>> Yes, we will only put bh internally allocated. >>> If bh is reserved in advance, we will not put it and re-assign it to NULL. >>> >> So this branch won't have risk, right? > Sorry... I'm not sure if I understand you correctly. > This branch will be walked through when previous part of bhs[] faces a read > failure in order to put bh allocated in ocfs2_read_blocks(). > And we assume all bh should be NULL or non-NULL, if new_bh is set, the back part > should also be put to release those buffer heads. > > If I made a mistake or misunderstand you, please let me know.I'm saying that sb_getblk() will only be called if bh hasn't been allocated yet. That means if it fails, the bh to be put can be guaranteed internal allocated. Also I don't think the WARN check is necessary as this is common path and will bring additional cpu consumption. We can make it clear at comments of ocfs2_read_blocks() that either all NULL or non-NULL bhs is prerequisite for the caller. And then we make sure we won't put bh that is allocated outside. Thanks, Joseph