Message ID | 20240710191118.40431-2-kovalev@altlinux.org (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | bfs: fix null-ptr-deref and possible warning in bfs_move_block() func | expand |
> Add a check to ensure 'sb_getblk' did not return NULL before copying data. Wording suggestion: that a sb_getblk() call How do you think about to use a summary phrase like “Prevent null pointer dereference in bfs_move_block()”? … > +++ b/fs/bfs/file.c > @@ -35,16 +35,22 @@ static int bfs_move_block(unsigned long from, unsigned long to, > struct super_block *sb) > { > struct buffer_head *bh, *new; > + int err; Can a statement (like the following) become more appropriate for such a function implementation? int ret = 0; Regards, Markus
10.07.2024 23:09, Markus Elfring wrote: >> Add a check to ensure 'sb_getblk' did not return NULL before copying data. > > Wording suggestion: > that a sb_getblk() call > > > How do you think about to use a summary phrase like > “Prevent null pointer dereference in bfs_move_block()”? Ok, I'll change it in the next version: bfs: prevent null pointer dereference in bfs_move_block() Add a check to ensure that a sb_getblk() call did not return NULL before copying data. > > … >> +++ b/fs/bfs/file.c >> @@ -35,16 +35,22 @@ static int bfs_move_block(unsigned long from, unsigned long to, >> struct super_block *sb) >> { >> struct buffer_head *bh, *new; >> + int err; > > Can a statement (like the following) become more appropriate for such > a function implementation? > > int ret = 0; Yes, thank you. > > Regards, > Markus
> Add a check to ensure that a sb_getblk() call did not return NULL before copying data.
How do you think about another refinement for such a change description?
Detect a failed sb_getblk() call (before copying data)
so that null pointer dereferences should not happen any more.
Regards,
Markus
Hi, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on v6.10-rc7 next-20240711] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/kovalev-altlinux-org/bfs-fix-null-ptr-deref-in-bfs_move_block/20240711-072644 base: linus/master patch link: https://lore.kernel.org/r/20240710191118.40431-2-kovalev%40altlinux.org patch subject: [PATCH fs/bfs 1/2] bfs: fix null-ptr-deref in bfs_move_block config: arm-randconfig-001-20240711 (https://download.01.org/0day-ci/archive/20240712/202407120052.Al11h5ur-lkp@intel.com/config) compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project a0c6b8aef853eedaa0980f07c0a502a5a8a9740e) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240712/202407120052.Al11h5ur-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202407120052.Al11h5ur-lkp@intel.com/ All warnings (new ones prefixed by >>): In file included from fs/bfs/file.c:15: In file included from include/linux/buffer_head.h:12: In file included from include/linux/blk_types.h:10: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:8: In file included from include/linux/cacheflush.h:5: In file included from arch/arm/include/asm/cacheflush.h:10: In file included from include/linux/mm.h:2258: include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] 514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" | ~~~~~~~~~~~ ^ ~~~ >> fs/bfs/file.c:44:6: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] 44 | if (unlikely(!new)) { | ^~~~~~~~~~~~~~ include/linux/compiler.h:77:22: note: expanded from macro 'unlikely' 77 | # define unlikely(x) __builtin_expect(!!(x), 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~ fs/bfs/file.c:53:9: note: uninitialized use occurs here 53 | return err; | ^~~ fs/bfs/file.c:44:2: note: remove the 'if' if its condition is always true 44 | if (unlikely(!new)) { | ^~~~~~~~~~~~~~~~~~~ fs/bfs/file.c:38:9: note: initialize the variable 'err' to silence this warning 38 | int err; | ^ | = 0 2 warnings generated. vim +44 fs/bfs/file.c 33 34 static int bfs_move_block(unsigned long from, unsigned long to, 35 struct super_block *sb) 36 { 37 struct buffer_head *bh, *new; 38 int err; 39 40 bh = sb_bread(sb, from); 41 if (!bh) 42 return -EIO; 43 new = sb_getblk(sb, to); > 44 if (unlikely(!new)) { 45 err = -EIO; 46 goto out_err_new; 47 } 48 memcpy(new->b_data, bh->b_data, bh->b_size); 49 mark_buffer_dirty(new); 50 brelse(new); 51 out_err_new: 52 bforget(bh); 53 return err; 54 } 55
diff --git a/fs/bfs/file.c b/fs/bfs/file.c index a778411574a96b..cb41ca2a2854e4 100644 --- a/fs/bfs/file.c +++ b/fs/bfs/file.c @@ -35,16 +35,22 @@ static int bfs_move_block(unsigned long from, unsigned long to, struct super_block *sb) { struct buffer_head *bh, *new; + int err; bh = sb_bread(sb, from); if (!bh) return -EIO; new = sb_getblk(sb, to); + if (unlikely(!new)) { + err = -EIO; + goto out_err_new; + } memcpy(new->b_data, bh->b_data, bh->b_size); mark_buffer_dirty(new); - bforget(bh); brelse(new); - return 0; +out_err_new: + bforget(bh); + return err; } static int bfs_move_blocks(struct super_block *sb, unsigned long start,