* [PATCH 0/2] eliminate the uninitialized compilation warning
@ 2024-04-19 7:48 Hongbo Li
2024-04-19 7:48 ` [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots Hongbo Li
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Hongbo Li @ 2024-04-19 7:48 UTC (permalink / raw)
To: kent.overstreet, bfoster; +Cc: linux-bcachefs, lihongbo22
Eliminating the uninitialized compilation warning in bcachefs-tools.
Hongbo Li (2):
bcachefs: eliminate the uninitialized compilation warning in
bch2_reconstruct_snapshots
bcachefs: eliminate the uninitialized compilation warning in
__do_six_trylock
fs/bcachefs/six.c | 6 ++----
fs/bcachefs/snapshot.c | 5 ++++-
2 files changed, 6 insertions(+), 5 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots
2024-04-19 7:48 [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li
@ 2024-04-19 7:48 ` Hongbo Li
2024-04-25 3:34 ` Kent Overstreet
2024-04-19 7:48 ` [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock Hongbo Li
2024-04-24 1:14 ` [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li
2 siblings, 1 reply; 9+ messages in thread
From: Hongbo Li @ 2024-04-19 7:48 UTC (permalink / raw)
To: kent.overstreet, bfoster; +Cc: linux-bcachefs, lihongbo22
[BUG]
When compiling the bcachefs-tools, the following compilation warning
is reported:
libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’:
libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized]
915 | snapshot->v.tree = cpu_to_le32(tree_id);
libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here
903 | u32 tree_id;
| ^~~~~~~
[CAUSE]
This is a false alert, because @tree_id is changed in
bch2_snapshot_tree_create after it returns 0. And if this function
returns other value, @tree_id wouldn't be used. Thus there should
be nothing wrong in logical.
[FIX]
Although the report itself is a false alert, we can still make it more
explicit by:
- check the input parameter 'u32 *tree_id' with WARN_ON_ONCE
- initialize @tree_id to U32_MAX
- add extra WARN_ON_ONCE to make sure @tree_id is updated
Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes")
Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
fs/bcachefs/snapshot.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c
index 0b26dee17a5a..cad3408903b2 100644
--- a/fs/bcachefs/snapshot.c
+++ b/fs/bcachefs/snapshot.c
@@ -78,6 +78,7 @@ __bch2_snapshot_tree_create(struct btree_trans *trans)
static int bch2_snapshot_tree_create(struct btree_trans *trans,
u32 root_id, u32 subvol_id, u32 *tree_id)
{
+ WARN_ON_ONCE(!tree_id);
struct bkey_i_snapshot_tree *n_tree =
__bch2_snapshot_tree_create(trans);
@@ -900,7 +901,7 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
if (bch2_snapshot_equiv(c, id))
return 0;
- u32 tree_id;
+ u32 tree_id = U32_MAX;
int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id);
if (ret)
return ret;
@@ -910,6 +911,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
if (ret)
return ret;
+ /* bch2_snapshot_tree_create returned 0, @tree_id must be updated. */
+ WARN_ON_ONCE(tree_id == U32_MAX);
bkey_snapshot_init(&snapshot->k_i);
snapshot->k.p = POS(0, id);
snapshot->v.tree = cpu_to_le32(tree_id);
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock
2024-04-19 7:48 [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li
2024-04-19 7:48 ` [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots Hongbo Li
@ 2024-04-19 7:48 ` Hongbo Li
2024-04-25 3:39 ` Kent Overstreet
2024-04-24 1:14 ` [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li
2 siblings, 1 reply; 9+ messages in thread
From: Hongbo Li @ 2024-04-19 7:48 UTC (permalink / raw)
To: kent.overstreet, bfoster; +Cc: linux-bcachefs, lihongbo22
When compiling the bcachefs-tools, the following compilation warning
is reported:
libbcachefs/six.c: In function ‘__do_six_trylock’:
libbcachefs/six.c:90:12: warning: ‘old’ may be used uninitialized in this function [-Wmaybe-uninitialized]
90 | if (!(old & SIX_LOCK_HELD_intent)) {
| ~~~~~^~~~~~~~~~~~~~~~~~~~~~~
This is also a false altert. Only when @type=SIX_LOCK_write and @try=false
are passed in __do_six_trylock, the second condition branch would enter
which does not initialize the @old variable. But six_set_owner will not
use @old if @type is not SIX_LOCK_intent. There should be nothing wrong
in logical too.
Although the report itself is a false alert, we can elimate the unitialize
compilation warning by assigning @old in front.
Fixes: 84a37cbf62e0 ("six locks: Wakeup now takes lock on behalf of waiter")
Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
fs/bcachefs/six.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/bcachefs/six.c b/fs/bcachefs/six.c
index 3a494c5d1247..9f782e4e3ca9 100644
--- a/fs/bcachefs/six.c
+++ b/fs/bcachefs/six.c
@@ -118,11 +118,11 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
struct task_struct *task, bool try)
{
int ret;
- u32 old;
+ u32 old = atomic_read(&lock->state);
EBUG_ON(type == SIX_LOCK_write && lock->owner != task);
EBUG_ON(type == SIX_LOCK_write &&
- (try != !(atomic_read(&lock->state) & SIX_LOCK_HELD_write)));
+ (try != !(old & SIX_LOCK_HELD_write)));
/*
* Percpu reader mode:
@@ -157,7 +157,6 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
smp_mb();
- old = atomic_read(&lock->state);
ret = !(old & l[type].lock_fail);
this_cpu_sub(*lock->readers, !ret);
@@ -182,7 +181,6 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
ret = -1 - SIX_LOCK_read;
}
} else {
- old = atomic_read(&lock->state);
do {
ret = !(old & l[type].lock_fail);
if (!ret || (type == SIX_LOCK_write && !try)) {
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] eliminate the uninitialized compilation warning
2024-04-19 7:48 [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li
2024-04-19 7:48 ` [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots Hongbo Li
2024-04-19 7:48 ` [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock Hongbo Li
@ 2024-04-24 1:14 ` Hongbo Li
2 siblings, 0 replies; 9+ messages in thread
From: Hongbo Li @ 2024-04-24 1:14 UTC (permalink / raw)
To: kent.overstreet, bfoster; +Cc: linux-bcachefs
These are two compilation warning in bcachefs-tools, the patches may be
useful.
Thanks,
Hongbo.
On 2024/4/19 15:48, Hongbo Li wrote:
> Eliminating the uninitialized compilation warning in bcachefs-tools.
>
> Hongbo Li (2):
> bcachefs: eliminate the uninitialized compilation warning in
> bch2_reconstruct_snapshots
> bcachefs: eliminate the uninitialized compilation warning in
> __do_six_trylock
>
> fs/bcachefs/six.c | 6 ++----
> fs/bcachefs/snapshot.c | 5 ++++-
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots
2024-04-19 7:48 ` [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots Hongbo Li
@ 2024-04-25 3:34 ` Kent Overstreet
2024-04-25 3:55 ` Hongbo Li
0 siblings, 1 reply; 9+ messages in thread
From: Kent Overstreet @ 2024-04-25 3:34 UTC (permalink / raw)
To: Hongbo Li; +Cc: bfoster, linux-bcachefs
On Fri, Apr 19, 2024 at 03:48:50PM +0800, Hongbo Li wrote:
> [BUG]
> When compiling the bcachefs-tools, the following compilation warning
> is reported:
> libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’:
> libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 915 | snapshot->v.tree = cpu_to_le32(tree_id);
> libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here
> 903 | u32 tree_id;
> | ^~~~~~~
>
> [CAUSE]
> This is a false alert, because @tree_id is changed in
> bch2_snapshot_tree_create after it returns 0. And if this function
> returns other value, @tree_id wouldn't be used. Thus there should
> be nothing wrong in logical.
>
> [FIX]
> Although the report itself is a false alert, we can still make it more
> explicit by:
> - check the input parameter 'u32 *tree_id' with WARN_ON_ONCE
> - initialize @tree_id to U32_MAX
> - add extra WARN_ON_ONCE to make sure @tree_id is updated
>
> Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes")
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
> fs/bcachefs/snapshot.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c
> index 0b26dee17a5a..cad3408903b2 100644
> --- a/fs/bcachefs/snapshot.c
> +++ b/fs/bcachefs/snapshot.c
> @@ -78,6 +78,7 @@ __bch2_snapshot_tree_create(struct btree_trans *trans)
> static int bch2_snapshot_tree_create(struct btree_trans *trans,
> u32 root_id, u32 subvol_id, u32 *tree_id)
> {
> + WARN_ON_ONCE(!tree_id);
There's no point checking for a null pointer like that; the oops from a
null ptr deref gives us exactly the same information.
> struct bkey_i_snapshot_tree *n_tree =
> __bch2_snapshot_tree_create(trans);
>
> @@ -900,7 +901,7 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
> if (bch2_snapshot_equiv(c, id))
> return 0;
>
> - u32 tree_id;
> + u32 tree_id = U32_MAX;
Just initialize it to 0. 0 is an invalid tree ID, so it'll be caught by
snapshot_tree_invalid() if it's not set.
> int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id);
> if (ret)
> return ret;
> @@ -910,6 +911,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
> if (ret)
> return ret;
>
> + /* bch2_snapshot_tree_create returned 0, @tree_id must be updated. */
> + WARN_ON_ONCE(tree_id == U32_MAX);
Nix this as well.
> bkey_snapshot_init(&snapshot->k_i);
> snapshot->k.p = POS(0, id);
> snapshot->v.tree = cpu_to_le32(tree_id);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock
2024-04-19 7:48 ` [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock Hongbo Li
@ 2024-04-25 3:39 ` Kent Overstreet
0 siblings, 0 replies; 9+ messages in thread
From: Kent Overstreet @ 2024-04-25 3:39 UTC (permalink / raw)
To: Hongbo Li; +Cc: bfoster, linux-bcachefs
On Fri, Apr 19, 2024 at 03:48:51PM +0800, Hongbo Li wrote:
> When compiling the bcachefs-tools, the following compilation warning
> is reported:
> libbcachefs/six.c: In function ‘__do_six_trylock’:
> libbcachefs/six.c:90:12: warning: ‘old’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 90 | if (!(old & SIX_LOCK_HELD_intent)) {
> | ~~~~~^~~~~~~~~~~~~~~~~~~~~~~
>
> This is also a false altert. Only when @type=SIX_LOCK_write and @try=false
> are passed in __do_six_trylock, the second condition branch would enter
> which does not initialize the @old variable. But six_set_owner will not
> use @old if @type is not SIX_LOCK_intent. There should be nothing wrong
> in logical too.
>
> Although the report itself is a false alert, we can elimate the unitialize
> compilation warning by assigning @old in front.
>
> Fixes: 84a37cbf62e0 ("six locks: Wakeup now takes lock on behalf of waiter")
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
> fs/bcachefs/six.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/bcachefs/six.c b/fs/bcachefs/six.c
> index 3a494c5d1247..9f782e4e3ca9 100644
> --- a/fs/bcachefs/six.c
> +++ b/fs/bcachefs/six.c
> @@ -118,11 +118,11 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
> struct task_struct *task, bool try)
> {
> int ret;
> - u32 old;
> + u32 old = atomic_read(&lock->state);
>
> EBUG_ON(type == SIX_LOCK_write && lock->owner != task);
> EBUG_ON(type == SIX_LOCK_write &&
> - (try != !(atomic_read(&lock->state) & SIX_LOCK_HELD_write)));
> + (try != !(old & SIX_LOCK_HELD_write)));
>
> /*
> * Percpu reader mode:
> @@ -157,7 +157,6 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
>
> smp_mb();
>
> - old = atomic_read(&lock->state);
Nope, this is wrong. That smp_mb() is there for a reason.
> ret = !(old & l[type].lock_fail);
>
> this_cpu_sub(*lock->readers, !ret);
> @@ -182,7 +181,6 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type,
> ret = -1 - SIX_LOCK_read;
> }
> } else {
> - old = atomic_read(&lock->state);
> do {
> ret = !(old & l[type].lock_fail);
> if (!ret || (type == SIX_LOCK_write && !try)) {
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots
2024-04-25 3:34 ` Kent Overstreet
@ 2024-04-25 3:55 ` Hongbo Li
2024-04-25 17:08 ` Kent Overstreet
0 siblings, 1 reply; 9+ messages in thread
From: Hongbo Li @ 2024-04-25 3:55 UTC (permalink / raw)
To: Kent Overstreet; +Cc: bfoster, linux-bcachefs
On 2024/4/25 11:34, Kent Overstreet wrote:
> On Fri, Apr 19, 2024 at 03:48:50PM +0800, Hongbo Li wrote:
>> [BUG]
>> When compiling the bcachefs-tools, the following compilation warning
>> is reported:
>> libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’:
>> libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>> 915 | snapshot->v.tree = cpu_to_le32(tree_id);
>> libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here
>> 903 | u32 tree_id;
>> | ^~~~~~~
>>
>> [CAUSE]
>> This is a false alert, because @tree_id is changed in
>> bch2_snapshot_tree_create after it returns 0. And if this function
>> returns other value, @tree_id wouldn't be used. Thus there should
>> be nothing wrong in logical.
>>
>> [FIX]
>> Although the report itself is a false alert, we can still make it more
>> explicit by:
>> - check the input parameter 'u32 *tree_id' with WARN_ON_ONCE
>> - initialize @tree_id to U32_MAX
>> - add extra WARN_ON_ONCE to make sure @tree_id is updated
>>
>> Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes")
>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
>> ---
>> fs/bcachefs/snapshot.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c
>> index 0b26dee17a5a..cad3408903b2 100644
>> --- a/fs/bcachefs/snapshot.c
>> +++ b/fs/bcachefs/snapshot.c
>> @@ -78,6 +78,7 @@ __bch2_snapshot_tree_create(struct btree_trans *trans)
>> static int bch2_snapshot_tree_create(struct btree_trans *trans,
>> u32 root_id, u32 subvol_id, u32 *tree_id)
>> {
>> + WARN_ON_ONCE(!tree_id);
>
> There's no point checking for a null pointer like that; the oops from a
> null ptr deref gives us exactly the same information.
>
>> struct bkey_i_snapshot_tree *n_tree =
>> __bch2_snapshot_tree_create(trans);
>>
>> @@ -900,7 +901,7 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
>> if (bch2_snapshot_equiv(c, id))
>> return 0;
>>
>> - u32 tree_id;
>> + u32 tree_id = U32_MAX;
>
> Just initialize it to 0. 0 is an invalid tree ID, so it'll be caught by
> snapshot_tree_invalid() if it's not set.
>
>> int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id);
>> if (ret)
>> return ret;
>> @@ -910,6 +911,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
>> if (ret)
>> return ret;
>>
>> + /* bch2_snapshot_tree_create returned 0, @tree_id must be updated. */
>> + WARN_ON_ONCE(tree_id == U32_MAX);
>
> Nix this as well.
This is to ensure that the tree_id is modified.
>
>> bkey_snapshot_init(&snapshot->k_i);
>> snapshot->k.p = POS(0, id);
>> snapshot->v.tree = cpu_to_le32(tree_id);
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots
2024-04-25 3:55 ` Hongbo Li
@ 2024-04-25 17:08 ` Kent Overstreet
2024-04-26 1:31 ` Hongbo Li
0 siblings, 1 reply; 9+ messages in thread
From: Kent Overstreet @ 2024-04-25 17:08 UTC (permalink / raw)
To: Hongbo Li; +Cc: bfoster, linux-bcachefs
On Thu, Apr 25, 2024 at 11:55:02AM +0800, Hongbo Li wrote:
>
>
> On 2024/4/25 11:34, Kent Overstreet wrote:
> > On Fri, Apr 19, 2024 at 03:48:50PM +0800, Hongbo Li wrote:
> > > [BUG]
> > > When compiling the bcachefs-tools, the following compilation warning
> > > is reported:
> > > libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’:
> > > libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > 915 | snapshot->v.tree = cpu_to_le32(tree_id);
> > > libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here
> > > 903 | u32 tree_id;
> > > | ^~~~~~~
> > >
> > > [CAUSE]
> > > This is a false alert, because @tree_id is changed in
> > > bch2_snapshot_tree_create after it returns 0. And if this function
> > > returns other value, @tree_id wouldn't be used. Thus there should
> > > be nothing wrong in logical.
> > >
> > > [FIX]
> > > Although the report itself is a false alert, we can still make it more
> > > explicit by:
> > > - check the input parameter 'u32 *tree_id' with WARN_ON_ONCE
> > > - initialize @tree_id to U32_MAX
> > > - add extra WARN_ON_ONCE to make sure @tree_id is updated
> > >
> > > Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes")
> > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> > > ---
> > > fs/bcachefs/snapshot.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c
> > > index 0b26dee17a5a..cad3408903b2 100644
> > > --- a/fs/bcachefs/snapshot.c
> > > +++ b/fs/bcachefs/snapshot.c
> > > @@ -78,6 +78,7 @@ __bch2_snapshot_tree_create(struct btree_trans *trans)
> > > static int bch2_snapshot_tree_create(struct btree_trans *trans,
> > > u32 root_id, u32 subvol_id, u32 *tree_id)
> > > {
> > > + WARN_ON_ONCE(!tree_id);
> >
> > There's no point checking for a null pointer like that; the oops from a
> > null ptr deref gives us exactly the same information.
> >
> > > struct bkey_i_snapshot_tree *n_tree =
> > > __bch2_snapshot_tree_create(trans);
> > > @@ -900,7 +901,7 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
> > > if (bch2_snapshot_equiv(c, id))
> > > return 0;
> > > - u32 tree_id;
> > > + u32 tree_id = U32_MAX;
> >
> > Just initialize it to 0. 0 is an invalid tree ID, so it'll be caught by
> > snapshot_tree_invalid() if it's not set.
> >
> > > int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id);
> > > if (ret)
> > > return ret;
> > > @@ -910,6 +911,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
> > > if (ret)
> > > return ret;
> > > + /* bch2_snapshot_tree_create returned 0, @tree_id must be updated. */
> > > + WARN_ON_ONCE(tree_id == U32_MAX);
> >
> > Nix this as well.
> This is to ensure that the tree_id is modified.
It's still not a good assertion; it's checking something simple and
purely local, and we've got other checks that will fire later - it's not
guarding against undefined behaviour.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots
2024-04-25 17:08 ` Kent Overstreet
@ 2024-04-26 1:31 ` Hongbo Li
0 siblings, 0 replies; 9+ messages in thread
From: Hongbo Li @ 2024-04-26 1:31 UTC (permalink / raw)
To: Kent Overstreet; +Cc: bfoster, linux-bcachefs
On 2024/4/26 1:08, Kent Overstreet wrote:
> On Thu, Apr 25, 2024 at 11:55:02AM +0800, Hongbo Li wrote:
>>
>>
>> On 2024/4/25 11:34, Kent Overstreet wrote:
>>> On Fri, Apr 19, 2024 at 03:48:50PM +0800, Hongbo Li wrote:
>>>> [BUG]
>>>> When compiling the bcachefs-tools, the following compilation warning
>>>> is reported:
>>>> libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’:
>>>> libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>>> 915 | snapshot->v.tree = cpu_to_le32(tree_id);
>>>> libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here
>>>> 903 | u32 tree_id;
>>>> | ^~~~~~~
>>>>
>>>> [CAUSE]
>>>> This is a false alert, because @tree_id is changed in
>>>> bch2_snapshot_tree_create after it returns 0. And if this function
>>>> returns other value, @tree_id wouldn't be used. Thus there should
>>>> be nothing wrong in logical.
>>>>
>>>> [FIX]
>>>> Although the report itself is a false alert, we can still make it more
>>>> explicit by:
>>>> - check the input parameter 'u32 *tree_id' with WARN_ON_ONCE
>>>> - initialize @tree_id to U32_MAX
>>>> - add extra WARN_ON_ONCE to make sure @tree_id is updated
>>>>
>>>> Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes")
>>>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
>>>> ---
>>>> fs/bcachefs/snapshot.c | 5 ++++-
>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c
>>>> index 0b26dee17a5a..cad3408903b2 100644
>>>> --- a/fs/bcachefs/snapshot.c
>>>> +++ b/fs/bcachefs/snapshot.c
>>>> @@ -78,6 +78,7 @@ __bch2_snapshot_tree_create(struct btree_trans *trans)
>>>> static int bch2_snapshot_tree_create(struct btree_trans *trans,
>>>> u32 root_id, u32 subvol_id, u32 *tree_id)
>>>> {
>>>> + WARN_ON_ONCE(!tree_id);
>>>
>>> There's no point checking for a null pointer like that; the oops from a
>>> null ptr deref gives us exactly the same information.
>>>
>>>> struct bkey_i_snapshot_tree *n_tree =
>>>> __bch2_snapshot_tree_create(trans);
>>>> @@ -900,7 +901,7 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
>>>> if (bch2_snapshot_equiv(c, id))
>>>> return 0;
>>>> - u32 tree_id;
>>>> + u32 tree_id = U32_MAX;
>>>
>>> Just initialize it to 0. 0 is an invalid tree ID, so it'll be caught by
>>> snapshot_tree_invalid() if it's not set.
>>>
>>>> int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id);
>>>> if (ret)
>>>> return ret;
>>>> @@ -910,6 +911,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id)
>>>> if (ret)
>>>> return ret;
>>>> + /* bch2_snapshot_tree_create returned 0, @tree_id must be updated. */
>>>> + WARN_ON_ONCE(tree_id == U32_MAX);
>>>
>>> Nix this as well.
>> This is to ensure that the tree_id is modified.
>
> It's still not a good assertion; it's checking something simple and
> purely local, and we've got other checks that will fire later - it's not
> guarding against undefined behaviour.
>
Thank you, I will revise the code based on your reviews.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-04-26 1:32 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-19 7:48 [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li
2024-04-19 7:48 ` [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots Hongbo Li
2024-04-25 3:34 ` Kent Overstreet
2024-04-25 3:55 ` Hongbo Li
2024-04-25 17:08 ` Kent Overstreet
2024-04-26 1:31 ` Hongbo Li
2024-04-19 7:48 ` [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock Hongbo Li
2024-04-25 3:39 ` Kent Overstreet
2024-04-24 1:14 ` [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).