| 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,