[bug report] btrfs: abort transaction in the process_one_buffer() log tree walk callback

[bug report] btrfs: abort transaction in the process_one_buffer() log tree walk callback

[Dan Carpenter]

Hello Filipe Manana,

This is a semi-automatic email about new static checker warnings.

Commit f6f79221b128 ("btrfs: abort transaction in the
process_one_buffer() log tree walk callback") from Jul 16, 2025,
leads to the following Smatch complaint:

fs/btrfs/tree-log.c:377 process_one_buffer() warn: variable dereferenced before check 'trans' (see line 375)
fs/btrfs/tree-log.c:388 process_one_buffer() warn: variable dereferenced before check 'trans' (see line 375)

fs/btrfs/tree-log.c
   374		if (wc->pin) {
   375			ret = btrfs_pin_extent_for_log_replay(trans, eb);
                                                              ^^^^^
The patch adds a dereference

   376			if (ret) {
   377				if (trans)
                                    ^^^^^
And also this check which is too late.

   378					btrfs_abort_transaction(trans, ret);
   379				else

regards,
dan carpenter

Re: [bug report] btrfs: abort transaction in the process_one_buffer() log tree walk callback

[Filipe Manana]

On Fri, Aug 15, 2025 at 7:50 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Hello Filipe Manana,
>
> This is a semi-automatic email about new static checker warnings.
>
> Commit f6f79221b128 ("btrfs: abort transaction in the
> process_one_buffer() log tree walk callback") from Jul 16, 2025,
> leads to the following Smatch complaint:
>
> fs/btrfs/tree-log.c:377 process_one_buffer() warn: variable dereferenced before check 'trans' (see line 375)
> fs/btrfs/tree-log.c:388 process_one_buffer() warn: variable dereferenced before check 'trans' (see line 375)
>
> fs/btrfs/tree-log.c
>    374          if (wc->pin) {
>    375                  ret = btrfs_pin_extent_for_log_replay(trans, eb);
>                                                               ^^^^^
> The patch adds a dereference

False alarm. This already happened before that patch, we didn't check
if wc->trans was NULL before calling
btrfs_pin_extent_for_log_replay(), and that's fine because if wc->pin
is true then trans is not NULL.

Thanks.

>
>    376                  if (ret) {
>    377                          if (trans)
>                                     ^^^^^
> And also this check which is too late.
>
>    378                                  btrfs_abort_transaction(trans, ret);
>    379                          else
>
> regards,
> dan carpenter
>

Re: [bug report] btrfs: abort transaction in the process_one_buffer() log tree walk callback

[Dan Carpenter]

On Fri, Aug 15, 2025 at 09:28:34AM +0100, Filipe Manana wrote:
> On Fri, Aug 15, 2025 at 7:50 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > Hello Filipe Manana,
> >
> > This is a semi-automatic email about new static checker warnings.
> >
> > Commit f6f79221b128 ("btrfs: abort transaction in the
> > process_one_buffer() log tree walk callback") from Jul 16, 2025,
> > leads to the following Smatch complaint:
> >
> > fs/btrfs/tree-log.c:377 process_one_buffer() warn: variable dereferenced before check 'trans' (see line 375)
> > fs/btrfs/tree-log.c:388 process_one_buffer() warn: variable dereferenced before check 'trans' (see line 375)
> >
> > fs/btrfs/tree-log.c
> >    374          if (wc->pin) {
> >    375                  ret = btrfs_pin_extent_for_log_replay(trans, eb);
> >                                                               ^^^^^
> > The patch adds a dereference
> 
> False alarm. This already happened before that patch, we didn't check
> if wc->trans was NULL before calling
> btrfs_pin_extent_for_log_replay(), and that's fine because if wc->pin
> is true then trans is not NULL.
> 

To be honest, the deleting the NULL check is the most common way to fix
a "pointless NULL check after a dereference" warning...

regards,
dan carpenter

Re: [bug report] btrfs: abort transaction in the process_one_buffer() log tree walk callback

[Filipe Manana]

On Fri, Aug 15, 2025 at 10:13 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Fri, Aug 15, 2025 at 09:28:34AM +0100, Filipe Manana wrote:
> > On Fri, Aug 15, 2025 at 7:50 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > >
> > > Hello Filipe Manana,
> > >
> > > This is a semi-automatic email about new static checker warnings.
> > >
> > > Commit f6f79221b128 ("btrfs: abort transaction in the
> > > process_one_buffer() log tree walk callback") from Jul 16, 2025,
> > > leads to the following Smatch complaint:
> > >
> > > fs/btrfs/tree-log.c:377 process_one_buffer() warn: variable dereferenced before check 'trans' (see line 375)
> > > fs/btrfs/tree-log.c:388 process_one_buffer() warn: variable dereferenced before check 'trans' (see line 375)
> > >
> > > fs/btrfs/tree-log.c
> > >    374          if (wc->pin) {
> > >    375                  ret = btrfs_pin_extent_for_log_replay(trans, eb);
> > >                                                               ^^^^^
> > > The patch adds a dereference
> >
> > False alarm. This already happened before that patch, we didn't check
> > if wc->trans was NULL before calling
> > btrfs_pin_extent_for_log_replay(), and that's fine because if wc->pin
> > is true then trans is not NULL.
> >
>
> To be honest, the deleting the NULL check is the most common way to fix
> a "pointless NULL check after a dereference" warning...

Yes, the NULL checks can be removed inside that if block.
I sent earlier a patch to remove them and it will later be folded into
that commit:

https://lore.kernel.org/linux-btrfs/4629b5fcab544101e9b6f935a7856428abe2f56d.1755248327.git.fdmanana@suse.com/



>
> regards,
> dan carpenter
>