[Ocfs2-devel] [PATCH] ocfs2: don't evaluate buffer head to NULL managed by caller

[Joseph Qi]

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?

Thanks,
Joseph