Re: [PATCH] udf: propagate error from udf_fiiter_add_entry

Re: [PATCH] udf: propagate error from udf_fiiter_add_entry

[Tristan Madani]

On Mon, 20 Apr 2026, Jan Kara wrote:
> Thanks for the patch. Did you analyze how it happened that the directory
> iteration fixing up tags failed? Because if it fails like this, we have
> practically wiped the directory which is not great. So if we are missing
> some directory consistency check before we start converting it, it would be
> good to add it instead of just failing like this later.

Good point. The syzbot reproducer uses a crafted filesystem image where
the inline directory data passes inode loading but contains entries that
fail udf_verify_fi() during iteration (e.g., bad tagIdent or CRC length
mismatch).

The problem is that by the time we iterate in udf_expand_dir_adinicb()
to fix up tagLocation, we've already changed the allocation type and
moved the data to a new block. If the iteration then fails, the
directory is in an inconsistent state.

The right fix would be to validate the inline directory entries
BEFORE starting the conversion -- iterate once to verify all entries
pass udf_verify_fi(), then proceed with the block allocation and tag
fixup only if validation succeeds. That way we never start a
destructive conversion on data we can't fully iterate.

I can send a v2 with this approach: a pre-conversion validation pass
using udf_fiiter_init/advance (which calls udf_verify_fi internally),
and only proceed if the full iteration succeeds. The error propagation
from v1 would remain as a safety net in case anything goes wrong
despite the pre-check.

Would that be the approach you have in mind?

Thanks,
Tristan

Re: [PATCH] udf: propagate error from udf_fiiter_add_entry

[Jan Kara]

On Tue 21-04-26 10:43:21, Tristan Madani wrote:
> On Mon, 20 Apr 2026, Jan Kara wrote:
> > Thanks for the patch. Did you analyze how it happened that the directory
> > iteration fixing up tags failed? Because if it fails like this, we have
> > practically wiped the directory which is not great. So if we are missing
> > some directory consistency check before we start converting it, it would be
> > good to add it instead of just failing like this later.
> 
> Good point. The syzbot reproducer uses a crafted filesystem image where
> the inline directory data passes inode loading but contains entries that
> fail udf_verify_fi() during iteration (e.g., bad tagIdent or CRC length
> mismatch).
> 
> The problem is that by the time we iterate in udf_expand_dir_adinicb()
> to fix up tagLocation, we've already changed the allocation type and
> moved the data to a new block. If the iteration then fails, the
> directory is in an inconsistent state.
> 
> The right fix would be to validate the inline directory entries
> BEFORE starting the conversion -- iterate once to verify all entries
> pass udf_verify_fi(), then proceed with the block allocation and tag
> fixup only if validation succeeds. That way we never start a
> destructive conversion on data we can't fully iterate.
> 
> I can send a v2 with this approach: a pre-conversion validation pass
> using udf_fiiter_init/advance (which calls udf_verify_fi internally),
> and only proceed if the full iteration succeeds. The error propagation
> from v1 would remain as a safety net in case anything goes wrong
> despite the pre-check.
> 
> Would that be the approach you have in mind?

Apparently you are running an AI agent to do the communication and coding.
That should be disclosed in Assisted-by: tag. Also I don't feel like
explaining to LLM how it should be doing the work. I could just do it
myself in this case.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] udf: propagate error from udf_fiiter_add_entry

[Jan Kara]

Hi!

On Tue 21-04-26 16:23:24, Tristan wrote:
> Hi Jan,
> 
> You're interacting with a human (with good will and intent), not an LLM.
> 
> I do use AI tooling in my workflow (as I disclosed to Johannes), but the
> analysis above and the proposed approach are my own.

I'm sorry. The tone of your reply triggered my "I'm talking to LLM"
detector and I didn't feel like educating LLM about where it is wrong.
Below is an explanation what is problematic with your suggestion.

> Le mar. 21 avr. 2026 à 14:36, Jan Kara <jack@suse.cz> a écrit :
> > On Tue 21-04-26 10:43:21, Tristan Madani wrote:
> > > On Mon, 20 Apr 2026, Jan Kara wrote:
> > > > Thanks for the patch. Did you analyze how it happened that the
> > directory
> > > > iteration fixing up tags failed? Because if it fails like this, we have
> > > > practically wiped the directory which is not great. So if we are
> > missing
> > > > some directory consistency check before we start converting it, it
> > would be
> > > > good to add it instead of just failing like this later.
> > >
> > > Good point. The syzbot reproducer uses a crafted filesystem image where
> > > the inline directory data passes inode loading but contains entries that
> > > fail udf_verify_fi() during iteration (e.g., bad tagIdent or CRC length
> > > mismatch).
> > >
> > > The problem is that by the time we iterate in udf_expand_dir_adinicb()
> > > to fix up tagLocation, we've already changed the allocation type and
> > > moved the data to a new block. If the iteration then fails, the
> > > directory is in an inconsistent state.
> > >
> > > The right fix would be to validate the inline directory entries
> > > BEFORE starting the conversion -- iterate once to verify all entries
> > > pass udf_verify_fi(), then proceed with the block allocation and tag
> > > fixup only if validation succeeds. That way we never start a
> > > destructive conversion on data we can't fully iterate.
> > >
> > > I can send a v2 with this approach: a pre-conversion validation pass
> > > using udf_fiiter_init/advance (which calls udf_verify_fi internally),
> > > and only proceed if the full iteration succeeds. The error propagation
> > > from v1 would remain as a safety net in case anything goes wrong
> > > despite the pre-check.
> > >
> > > Would that be the approach you have in mind?

Not quite. udf_expand_dir_adinicb() is called only from
udf_fiiter_add_entry(). Before we get to calling udf_expand_dir_adinicb()
we must have already iterated the whole directory so at that point all
directory entries have passed udf_verify_fi(). The directory is locked all
the time so it is unclear how a few lines later udf_verify_fi() can fail in
iteration in udf_expand_dir_adinicb(). So this we need to understand first
before deciding which check should be added where...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH] udf: propagate error from udf_fiiter_add_entry

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

udf_fiiter_add_entry() triggers a WARN when it encounters an error
condition but does not propagate the error to callers. This can be
triggered by a corrupted filesystem image.

Replace the WARN with proper error propagation to allow callers to
handle the failure gracefully.

Reported-by: syzbot+969e250fc7983fc7417c@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 fs/udf/namei.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index 9a3b7cef36064..2249b79c0e0b2 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -207,14 +207,9 @@ static int udf_expand_dir_adinicb(struct inode *inode, udf_pblk_t *block)
 		udf_fiiter_write_fi(&iter, impuse);
 	}
 	brelse(dbh);
-	/*
-	 * We don't expect the iteration to fail as the directory has been
-	 * already verified to be correct
-	 */
-	WARN_ON_ONCE(ret);
 	udf_fiiter_release(&iter);
 
-	return 0;
+	return ret;
 }
 
 static int udf_fiiter_add_entry(struct inode *dir, struct dentry *dentry,
-- 
2.47.3

Re: [PATCH] udf: propagate error from udf_fiiter_add_entry

[Jan Kara]

On Sat 18-04-26 13:10:35, Tristan Madani wrote:
> From: Tristan Madani <tristan@talencesecurity.com>
> 
> udf_fiiter_add_entry() triggers a WARN when it encounters an error
> condition but does not propagate the error to callers. This can be
> triggered by a corrupted filesystem image.
> 
> Replace the WARN with proper error propagation to allow callers to
> handle the failure gracefully.
> 
> Reported-by: syzbot+969e250fc7983fc7417c@syzkaller.appspotmail.com
> Cc: stable@vger.kernel.org
> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>

Thanks for the patch. Did you analyze how it happened that the directory
iteration fixing up tags failed? Because if it fails like this, we have
practically wiped the directory which is not great. So if we are missing
some directory consistency check before we start converting it, it would be
good to add it instead of just failing like this later.

								Honza

> ---
>  fs/udf/namei.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/udf/namei.c b/fs/udf/namei.c
> index 9a3b7cef36064..2249b79c0e0b2 100644
> --- a/fs/udf/namei.c
> +++ b/fs/udf/namei.c
> @@ -207,14 +207,9 @@ static int udf_expand_dir_adinicb(struct inode *inode, udf_pblk_t *block)
>  		udf_fiiter_write_fi(&iter, impuse);
>  	}
>  	brelse(dbh);
> -	/*
> -	 * We don't expect the iteration to fail as the directory has been
> -	 * already verified to be correct
> -	 */
> -	WARN_ON_ONCE(ret);
>  	udf_fiiter_release(&iter);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int udf_fiiter_add_entry(struct inode *dir, struct dentry *dentry,
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR