From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joseph Qi Date: Fri, 30 Mar 2018 10:03:53 +0800 Subject: [Ocfs2-devel] [PATCH] ocfs2: don't evaluate buffer head to NULL managed by caller In-Reply-To: <63ADC13FD55D6546B7DECE290D39E373F2958D95@H3CMLB12-EX.srv.huawei-3com.com> References: <1522289162-31693-1-git-send-email-ge.changwei@h3c.com> <9e21437e-0c16-4bc7-2e87-a8310b2172ea@gmail.com> <63ADC13FD55D6546B7DECE290D39E373F2958D95@H3CMLB12-EX.srv.huawei-3com.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: ocfs2-devel@oss.oracle.com 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 >>> --- >>> 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