[syzbot] [block?] BUG: unable to handle kernel NULL pointer dereference in lo_rw_aio

[syzbot] [block?] BUG: unable to handle kernel NULL pointer dereference in lo_rw_aio

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    119009db2674 Merge tag 'vfs-6.15-rc3.fixes.2' of git://git..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=15c93fe4580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=7a7c679f880028f0
dashboard link: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=179aeccc580000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=100b5b98580000

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/7feb34a89c2a/non_bootable_disk-119009db.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/1cd1adb50b98/vmlinux-119009db.xz
kernel image: https://storage.googleapis.com/syzbot-assets/d1e790c57be7/bzImage-119009db.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com

BUG: kernel NULL pointer dereference, address: 0000000000000000
#PF: supervisor instruction fetch in kernel mode
#PF: error_code(0x0010) - not-present page
PGD 0 P4D 0 
Oops: Oops: 0010 [#1] SMP KASAN NOPTI
CPU: 0 UID: 0 PID: 12 Comm: kworker/u32:0 Not tainted 6.15.0-rc2-syzkaller-00471-g119009db2674 #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
Workqueue: loop8 loop_rootcg_workfn
RIP: 0010:0x0
Code: Unable to access opcode bytes at 0xffffffffffffffd6.
RSP: 0018:ffffc900000f7a38 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffffffff8beceec0 RCX: ffffffff86084265
RDX: 1ffffffff17d9ddd RSI: ffffc900000f7b28 RDI: ffff8880261b3128
RBP: ffff8880261b3128 R08: 0000000000000005 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000002be0 R12: ffffc900000f7b28
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff8880d69b2000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffd6 CR3: 000000000e180000 CR4: 0000000000352ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 lo_rw_aio.isra.0+0x9c2/0xd90 drivers/block/loop.c:393
 do_req_filebacked drivers/block/loop.c:424 [inline]
 loop_handle_cmd drivers/block/loop.c:1866 [inline]
 loop_process_work+0x8a4/0x10d0 drivers/block/loop.c:1901
 process_one_work+0x9cc/0x1b70 kernel/workqueue.c:3238
 process_scheduled_works kernel/workqueue.c:3319 [inline]
 worker_thread+0x6c8/0xf10 kernel/workqueue.c:3400
 kthread+0x3c2/0x780 kernel/kthread.c:464
 ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:153
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
 </TASK>
Modules linked in:
CR2: 0000000000000000
---[ end trace 0000000000000000 ]---
RIP: 0010:0x0
Code: Unable to access opcode bytes at 0xffffffffffffffd6.
RSP: 0018:ffffc900000f7a38 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffffffff8beceec0 RCX: ffffffff86084265
RDX: 1ffffffff17d9ddd RSI: ffffc900000f7b28 RDI: ffff8880261b3128
RBP: ffff8880261b3128 R08: 0000000000000005 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000002be0 R12: ffffc900000f7b28
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff8880d69b2000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffd6 CR3: 000000000e180000 CR4: 0000000000352ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Re: [syzbot] Re: [syzbot] [block?] BUG: unable to handle kernel NULL pointer dereference in lo_rw_aio

[syzbot]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org.

***

Subject: Re: [syzbot] [block?] BUG: unable to handle kernel NULL pointer dereference in lo_rw_aio
Author: lizhi.xu@windriver.com

selinux policy not support read_iter

#syz test

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 47480eb2189b..e71936c6d82d 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -484,6 +484,7 @@ static int sel_mmap_policy(struct file *filp, struct vm_area_struct *vma)
 static const struct file_operations sel_policy_ops = {
 	.open		= sel_open_policy,
 	.read		= sel_read_policy,
+	.read_iter      = generic_file_read_iter,
 	.mmap		= sel_mmap_policy,
 	.release	= sel_release_policy,
 	.llseek		= generic_file_llseek,

Re: [syzbot] Re: [syzbot] [block?] BUG: unable to handle kernel NULL pointer dereference in lo_rw_aio

[syzbot]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org.

***

Subject: Re: [syzbot] [block?] BUG: unable to handle kernel NULL pointer dereference in lo_rw_aio
Author: lizhi.xu@windriver.com

selinux policy not support read_iter

#syz test

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 47480eb2189b..2bf0d2b2f2ea 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -481,9 +481,15 @@ static int sel_mmap_policy(struct file *filp, struct vm_area_struct *vma)
 	return 0;
 }
 
+static ssize_t sel_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+	return 0;
+}
+
 static const struct file_operations sel_policy_ops = {
 	.open		= sel_open_policy,
 	.read		= sel_read_policy,
+	.read_iter	= sel_file_read_iter,
 	.mmap		= sel_mmap_policy,
 	.release	= sel_release_policy,
 	.llseek		= generic_file_llseek,

[PATCH] loop: Add sanity check for read/write_iter

[Lizhi Xu]

Some file systems do not support read_iter or write_iter, such as selinuxfs
in this issue.
So before calling them, first confirm that the interface is supported and
then call it.

Reported-by: syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 drivers/block/loop.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 674527d770dc..4f968e3071ed 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -449,10 +449,15 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
 	cmd->iocb.ki_flags = IOCB_DIRECT;
 	cmd->iocb.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
 
-	if (rw == ITER_SOURCE)
-		ret = file->f_op->write_iter(&cmd->iocb, &iter);
-	else
-		ret = file->f_op->read_iter(&cmd->iocb, &iter);
+	ret = 0;
+	if (rw == ITER_SOURCE) {
+		if (likely(file->f_op->write_iter))
+			ret = file->f_op->write_iter(&cmd->iocb, &iter);
+	}
+	else {
+		if (likely(file->f_op->read_iter))
+			ret = file->f_op->read_iter(&cmd->iocb, &iter);
+	}
 
 	lo_rw_aio_do_completion(cmd);
 
-- 
2.43.0

Re: [PATCH] loop: Add sanity check for read/write_iter

[Zhu Yanjun]

在 2025/4/25 5:40, Lizhi Xu 写道:
> Some file systems do not support read_iter or write_iter, such as selinuxfs
> in this issue.
> So before calling them, first confirm that the interface is supported and
> then call it.
> 
> Reported-by: syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>   drivers/block/loop.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 674527d770dc..4f968e3071ed 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -449,10 +449,15 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
>   	cmd->iocb.ki_flags = IOCB_DIRECT;
>   	cmd->iocb.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
>   
> -	if (rw == ITER_SOURCE)
> -		ret = file->f_op->write_iter(&cmd->iocb, &iter);
> -	else
> -		ret = file->f_op->read_iter(&cmd->iocb, &iter);
> +	ret = 0;
> +	if (rw == ITER_SOURCE) {
> +		if (likely(file->f_op->write_iter))
> +			ret = file->f_op->write_iter(&cmd->iocb, &iter);
> +	}
> +	else {
> +		if (likely(file->f_op->read_iter))

"else if" is better?

Zhu Yanjun
> +			ret = file->f_op->read_iter(&cmd->iocb, &iter);
> +	}
>   
>   	lo_rw_aio_do_completion(cmd);
>   

Re: [PATCH] loop: Add sanity check for read/write_iter

[Lizhi Xu]

On Fri, 25 Apr 2025 06:06:51 +0200, Zhu Yanjun wrote:
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 674527d770dc..4f968e3071ed 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -449,10 +449,15 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
> >       cmd->iocb.ki_flags = IOCB_DIRECT;
> >       cmd->iocb.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
> >
> > -     if (rw == ITER_SOURCE)
> > -             ret = file->f_op->write_iter(&cmd->iocb, &iter);
> > -     else
> > -             ret = file->f_op->read_iter(&cmd->iocb, &iter);
> > +     ret = 0;
> > +     if (rw == ITER_SOURCE) {
> > +             if (likely(file->f_op->write_iter))
> > +                     ret = file->f_op->write_iter(&cmd->iocb, &iter);
> > +     }
> > +     else {
> > +             if (likely(file->f_op->read_iter))
> 
> "else if" is better?
There is nothing wrong with writing it this way logically, but it will
destroy the clarity of the original context regarding the read/write logical
relationship.

Re: [PATCH] loop: Add sanity check for read/write_iter

[Ming Lei]

On Fri, Apr 25, 2025 at 11:41 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
>
> Some file systems do not support read_iter or write_iter, such as selinuxfs
> in this issue.
> So before calling them, first confirm that the interface is supported and
> then call it.
>
> Reported-by: syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>  drivers/block/loop.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 674527d770dc..4f968e3071ed 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -449,10 +449,15 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
>         cmd->iocb.ki_flags = IOCB_DIRECT;
>         cmd->iocb.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
>
> -       if (rw == ITER_SOURCE)
> -               ret = file->f_op->write_iter(&cmd->iocb, &iter);
> -       else
> -               ret = file->f_op->read_iter(&cmd->iocb, &iter);
> +       ret = 0;
> +       if (rw == ITER_SOURCE) {
> +               if (likely(file->f_op->write_iter))
> +                       ret = file->f_op->write_iter(&cmd->iocb, &iter);
> +       }
> +       else {
> +               if (likely(file->f_op->read_iter))
> +                       ret = file->f_op->read_iter(&cmd->iocb, &iter);
> +       }

The check can be added in loop_configure()/loop_change_fd()
instead of fast IO path.

Thanks,

Re: [PATCH] loop: Add sanity check for read/write_iter

[Lizhi Xu]

On Fri, 25 Apr 2025 12:20:21 +0800, Ming Lei <ming.lei@redhat.com> wrote:
> > Some file systems do not support read_iter or write_iter, such as selinuxfs
> > in this issue.
> > So before calling them, first confirm that the interface is supported and
> > then call it.
> >
> > Reported-by: syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > ---
> >  drivers/block/loop.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 674527d770dc..4f968e3071ed 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -449,10 +449,15 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
> >         cmd->iocb.ki_flags = IOCB_DIRECT;
> >         cmd->iocb.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
> >
> > -       if (rw == ITER_SOURCE)
> > -               ret = file->f_op->write_iter(&cmd->iocb, &iter);
> > -       else
> > -               ret = file->f_op->read_iter(&cmd->iocb, &iter);
> > +       ret = 0;
> > +       if (rw == ITER_SOURCE) {
> > +               if (likely(file->f_op->write_iter))
> > +                       ret = file->f_op->write_iter(&cmd->iocb, &iter);
> > +       }
> > +       else {
> > +               if (likely(file->f_op->read_iter))
> > +                       ret = file->f_op->read_iter(&cmd->iocb, &iter);
> > +       }
> 
> The check can be added in loop_configure()/loop_change_fd()
> instead of fast IO path.
Yes, you are right, I will test and send V2 patch.

BR,
Lizhi

Re: [syzbot] Re: [syzbot] [block?] BUG: unable to handle kernel NULL pointer dereference in lo_rw_aio

[syzbot]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org.

***

Subject: Re: [syzbot] [block?] BUG: unable to handle kernel NULL pointer dereference in lo_rw_aio
Author: lizhi.xu@windriver.com

selinux policy not support read_iter

#syz test

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 4f968e3071ed..3572b50dbf0a 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1044,6 +1044,10 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 
 	if (!file)
 		return -EBADF;
+
+	if (unlikely(!file->f_op->read_iter))
+		return -EINVAL;
+
 	is_loop = is_loop_device(file);
 
 	/* This is safe, since we have a reference from open(). */

[PATCH V2] loop: Add sanity check for read/write_iter

[Lizhi Xu]

Some file systems do not support read_iter or write_iter, such as selinuxfs
in this issue.
So before calling them, first confirm that the interface is supported and
then call it.

Reported-by: syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
V1 -> V2: move check to loop_configure and loop_change_fd

 drivers/block/loop.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 674527d770dc..d2651e3b5142 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -603,6 +603,12 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	if (!file)
 		return -EBADF;
 
+	if (unlikely(!file->f_op->read_iter))
+		return -EINVAL;
+
+	if (file->f_mode & FMODE_WRITE && unlikely(!file->f_op->write_iter))
+		return -EINVAL;
+
 	/* suppress uevents while reconfiguring the device */
 	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
 
@@ -1039,6 +1045,14 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 
 	if (!file)
 		return -EBADF;
+
+	if (unlikely(!file->f_op->read_iter))
+		return -EINVAL;
+
+	if (((file->f_mode & FMODE_WRITE) || (mode & BLK_OPEN_WRITE)) &&
+	    unlikely(!file->f_op->write_iter))
+		return -EINVAL;
+
 	is_loop = is_loop_device(file);
 
 	/* This is safe, since we have a reference from open(). */
-- 
2.43.0

Re: [PATCH V2] loop: Add sanity check for read/write_iter

[Christoph Hellwig]

On Fri, Apr 25, 2025 at 01:38:03PM +0800, Lizhi Xu wrote:
> Some file systems do not support read_iter or write_iter, such as selinuxfs
> in this issue.
> So before calling them, first confirm that the interface is supported and
> then call it.

Nit: commit messages should not have lines longer than 73 characters.

Please also add a:

Fixes: f2fed441c69b ("loop: stop using vfs_iter__{read,write} for buffered I/O")

and maybe add a blurb that vfs_iter_read/write had this check.

Now the other interesting bit is why we did not hit this earlier with
direct I/O?  I guess it's because we basically have no instances
supporting direct I/O and not using the iter ops.

> @@ -603,6 +603,12 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
>  	if (!file)
>  		return -EBADF;
>  
> +	if (unlikely(!file->f_op->read_iter))
> +		return -EINVAL;
> +
> +	if (file->f_mode & FMODE_WRITE && unlikely(!file->f_op->write_iter))
> +		return -EINVAL;

Can we have a common helper for change_fd and configure, please?

Please also drop the unlikelys - this is not a fast path and we don't
need to micro-optimize.

A bit unrelated, but loop-configure actually checks for write_iter
and forces read-only for that.  Do we need the same kind of check in
change_fd?

Re: [PATCH V2] loop: Add sanity check for read/write_iter

[Lizhi Xu]

On Fri, 25 Apr 2025 06:28:43 -0700, Christoph Hellwig wrote:
> > Some file systems do not support read_iter or write_iter, such as selinuxfs
> > in this issue.
> > So before calling them, first confirm that the interface is supported and
> > then call it.
> 
> Nit: commit messages should not have lines longer than 73 characters.
> 
> Please also add a:
> 
> Fixes: f2fed441c69b ("loop: stop using vfs_iter__{read,write} for buffered I/O")
OK, I would deal with both of the things you mentioned above.
> 
> and maybe add a blurb that vfs_iter_read/write had this check.
It makes no sence. The current issue context does not involve vfs layer
iter_read/write related routines.
> 
> Now the other interesting bit is why we did not hit this earlier with
> direct I/O?  I guess it's because we basically have no instances
> supporting direct I/O and not using the iter ops.
> 
> > @@ -603,6 +603,12 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
> >  	if (!file)
> >  		return -EBADF;
> >
> > +	if (unlikely(!file->f_op->read_iter))
> > +		return -EINVAL;
> > +
> > +	if (file->f_mode & FMODE_WRITE && unlikely(!file->f_op->write_iter))
> > +		return -EINVAL;
> 
> Can we have a common helper for change_fd and configure, please?
The common helper is not very meaningful for this case, but it may be
useful later, so it can be added.
> 
> Please also drop the unlikelys - this is not a fast path and we don't
> need to micro-optimize.
Yes, you are right, I will drop it.
> 
> A bit unrelated, but loop-configure actually checks for write_iter
> and forces read-only for that.  Do we need the same kind of check in
> change_fd?
In the context of this case, it is necessary to judge the write mode of
the new file.

[PATCH V3] loop: Add sanity check for read/write_iter

[Lizhi Xu]

Some file systems do not support read_iter/write_iter, such as selinuxfs
in this issue.
So before calling them, first confirm that the interface is supported and
then call it.

Fixes: f2fed441c69b ("loop: stop using vfs_iter__{read,write} for buffered I/O")
Reported-by: syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
V1 -> V2: move check to loop_configure and loop_change_fd
V2 -> V3: using helper for this check

 drivers/block/loop.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 674527d770dc..7b78ddf7b819 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -582,6 +582,19 @@ static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
 	lo->lo_min_dio_size = loop_query_min_dio_size(lo);
 }
 
+static int loop_check_backing_file(struct file *file, blk_mode_t mode, bool change)
+{
+	if (!file->f_op->read_iter)
+		return -EINVAL;
+
+	if (((file->f_mode & FMODE_WRITE) ||
+	     (!change && (mode & BLK_OPEN_WRITE))) &&
+	    (!file->f_op->write_iter))
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * loop_change_fd switched the backing store of a loopback device to
  * a new file. This is useful for operating system installers to free up
@@ -603,6 +616,10 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	if (!file)
 		return -EBADF;
 
+	error = loop_check_backing_file(file, 0, true);
+	if (error)
+		return error;
+
 	/* suppress uevents while reconfiguring the device */
 	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
 
@@ -1039,6 +1056,11 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 
 	if (!file)
 		return -EBADF;
+
+	error = loop_check_backing_file(file, mode, false);
+	if (error)
+		return error;
+
 	is_loop = is_loop_device(file);
 
 	/* This is safe, since we have a reference from open(). */
-- 
2.43.0

Re: [PATCH V2] loop: Add sanity check for read/write_iter

[Christoph Hellwig]

On Sat, Apr 26, 2025 at 09:50:28AM +0800, Lizhi Xu wrote:
> > and maybe add a blurb that vfs_iter_read/write had this check.
> It makes no sence. The current issue context does not involve vfs layer
> iter_read/write related routines.

Yes.  But explaining how a change caused a regression is good
information for a commit log.

Re: [PATCH V3] loop: Add sanity check for read/write_iter

[Christoph Hellwig]

On Sat, Apr 26, 2025 at 10:10:55AM +0800, Lizhi Xu wrote:
> +static int loop_check_backing_file(struct file *file, blk_mode_t mode, bool change)
> +{
> +	if (!file->f_op->read_iter)
> +		return -EINVAL;
> +
> +	if (((file->f_mode & FMODE_WRITE) ||
> +	     (!change && (mode & BLK_OPEN_WRITE))) &&
> +	    (!file->f_op->write_iter))
> +		return -EINVAL;

This looks a bit odd.  Both callers have the open struct file, so
we should be able to check f_mode for both cases and not need the
change flag as far as I can tell.  Or did I miss something/

If for some reason we could not pass the fmode, the helper is
probably not all that useful.

Re: [PATCH V3] loop: Add sanity check for read/write_iter

[Lizhi Xu]

On Mon, 28 Apr 2025 05:49:20 -0700, Christoph Hellwig wrote:
> > +static int loop_check_backing_file(struct file *file, blk_mode_t mode, bool change)
> > +{
> > +	if (!file->f_op->read_iter)
> > +		return -EINVAL;
> > +
> > +	if (((file->f_mode & FMODE_WRITE) ||
> > +	     (!change && (mode & BLK_OPEN_WRITE))) &&
> > +	    (!file->f_op->write_iter))
> > +		return -EINVAL;
> 
> This looks a bit odd.  Both callers have the open struct file, so
> we should be able to check f_mode for both cases and not need the
> change flag as far as I can tell.  Or did I miss something/
Changing flags? What are you talking about?
This patch is to fix filesystems that are missing read_iter or wirte_iter
support.
> 
> If for some reason we could not pass the fmode, the helper is
> probably not all that useful.
The helper function does not pass fmode, but passes 'blk_mode_t mode',
because it is used when executing LOOP_SET_FD or LOOP_CONFIGURE, but not
when executing LOOP_CHANGE_FD.
I think the purpose of this helper function is just to facilitate code
management and facilitate similar problems later.

Re: [PATCH V2] loop: Add sanity check for read/write_iter

[Lizhi Xu]

On Mon, 28 Apr 2025 05:46:25 -0700, Christoph Hellwig wrote:
> > > and maybe add a blurb that vfs_iter_read/write had this check.
> > It makes no sence. The current issue context does not involve vfs layer
> > iter_read/write related routines.
> 
> Yes.  But explaining how a change caused a regression is good
> information for a commit log.
What changes?
The check in vfs_iter_read/write is not relevant to this case.
It is best to not write something irrelevant.

Re: [PATCH V3] loop: Add sanity check for read/write_iter

[Christoph Hellwig]

On Mon, Apr 28, 2025 at 09:42:31PM +0800, Lizhi Xu wrote:
> On Mon, 28 Apr 2025 05:49:20 -0700, Christoph Hellwig wrote:
> > > +static int loop_check_backing_file(struct file *file, blk_mode_t mode, bool change)
> > > +{
> > > +	if (!file->f_op->read_iter)
> > > +		return -EINVAL;
> > > +
> > > +	if (((file->f_mode & FMODE_WRITE) ||
> > > +	     (!change && (mode & BLK_OPEN_WRITE))) &&
> > > +	    (!file->f_op->write_iter))
> > > +		return -EINVAL;
> > 
> > This looks a bit odd.  Both callers have the open struct file, so
> > we should be able to check f_mode for both cases and not need the
> > change flag as far as I can tell.  Or did I miss something/
> Changing flags? What are you talking about?

About the 'bool change' function argument used as a flag.

> The helper function does not pass fmode, but passes 'blk_mode_t mode',
> because it is used when executing LOOP_SET_FD or LOOP_CONFIGURE, but not
> when executing LOOP_CHANGE_FD.
> I think the purpose of this helper function is just to facilitate code
> management and facilitate similar problems later.

But you can just check file->f_mode unconditionally instead of passing
the blk_mode_t.  The BLK_OPEN_WRITE check is only needed for force
the read-only flag separately, and can be kept in the caller before
the call to the helper.

Re: [PATCH V2] loop: Add sanity check for read/write_iter

[Christoph Hellwig]

On Mon, Apr 28, 2025 at 09:48:12PM +0800, Lizhi Xu wrote:
> On Mon, 28 Apr 2025 05:46:25 -0700, Christoph Hellwig wrote:
> > > > and maybe add a blurb that vfs_iter_read/write had this check.
> > > It makes no sence. The current issue context does not involve vfs layer
> > > iter_read/write related routines.
> > 
> > Yes.  But explaining how a change caused a regression is good
> > information for a commit log.
> What changes?
> The check in vfs_iter_read/write is not relevant to this case.
> It is best to not write something irrelevant.

It is releavant in that vfs_iter_read/write have the check, and removal
of their used caused szybot to be able to hit this issue.

[PATCH V4] loop: Add sanity check for read/write_iter

[Lizhi Xu]

Some file systems do not support read_iter/write_iter, such as selinuxfs
in this issue.
So before calling them, first confirm that the interface is supported and
then call it.

It is releavant in that vfs_iter_read/write have the check, and removal
of their used caused szybot to be able to hit this issue.

Fixes: f2fed441c69b ("loop: stop using vfs_iter__{read,write} for buffered I/O")
Reported-by: syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
V1 -> V2: move check to loop_configure and loop_change_fd
V2 -> V3: using helper for this check
V3 -> V4: remove input parameters change and mode

 drivers/block/loop.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 46cba261075f..655d33e63cb9 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -505,6 +505,17 @@ static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
 	lo->lo_min_dio_size = loop_query_min_dio_size(lo);
 }
 
+static int loop_check_backing_file(struct file *file)
+{
+	if (!file->f_op->read_iter)
+		return -EINVAL;
+
+	if ((file->f_mode & FMODE_WRITE) && (!file->f_op->write_iter))
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * loop_change_fd switched the backing store of a loopback device to
  * a new file. This is useful for operating system installers to free up
@@ -526,6 +537,10 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	if (!file)
 		return -EBADF;
 
+	error = loop_check_backing_file(file);
+	if (error)
+		return error;
+
 	/* suppress uevents while reconfiguring the device */
 	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
 
@@ -963,6 +978,14 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 
 	if (!file)
 		return -EBADF;
+
+	if ((mode & BLK_OPEN_WRITE) && (!file->f_op->write_iter))
+		return -EINVAL;
+
+	error = loop_check_backing_file(file);
+	if (error)
+		return error;
+
 	is_loop = is_loop_device(file);
 
 	/* This is safe, since we have a reference from open(). */
-- 
2.43.0

Re: [PATCH V4] loop: Add sanity check for read/write_iter

[Christoph Hellwig]

On Mon, Apr 28, 2025 at 10:15:44PM +0800, Lizhi Xu wrote:
> +	if ((file->f_mode & FMODE_WRITE) && (!file->f_op->write_iter))

No need for the braces around !file->f_op->write_iter.

> +	if ((mode & BLK_OPEN_WRITE) && (!file->f_op->write_iter))

Same here.  Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH V5] loop: Add sanity check for read/write_iter

[Lizhi Xu]

Some file systems do not support read_iter/write_iter, such as selinuxfs
in this issue.
So before calling them, first confirm that the interface is supported and
then call it.

It is releavant in that vfs_iter_read/write have the check, and removal
of their used caused szybot to be able to hit this issue.

Fixes: f2fed441c69b ("loop: stop using vfs_iter__{read,write} for buffered I/O")
Reported-by: syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
V1 -> V2: move check to loop_configure and loop_change_fd
V2 -> V3: using helper for this check
V3 -> V4: remove input parameters change and mode
V4 -> V5: remove braces around !file->f_op->write_iter

 drivers/block/loop.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 46cba261075f..655d33e63cb9 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -505,6 +505,17 @@ static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
 	lo->lo_min_dio_size = loop_query_min_dio_size(lo);
 }
 
+static int loop_check_backing_file(struct file *file)
+{
+	if (!file->f_op->read_iter)
+		return -EINVAL;
+
+	if ((file->f_mode & FMODE_WRITE) && !file->f_op->write_iter)
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * loop_change_fd switched the backing store of a loopback device to
  * a new file. This is useful for operating system installers to free up
@@ -526,6 +537,10 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	if (!file)
 		return -EBADF;
 
+	error = loop_check_backing_file(file);
+	if (error)
+		return error;
+
 	/* suppress uevents while reconfiguring the device */
 	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
 
@@ -963,6 +978,14 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 
 	if (!file)
 		return -EBADF;
+
+	if ((mode & BLK_OPEN_WRITE) && !file->f_op->write_iter)
+		return -EINVAL;
+
+	error = loop_check_backing_file(file);
+	if (error)
+		return error;
+
 	is_loop = is_loop_device(file);
 
 	/* This is safe, since we have a reference from open(). */
-- 
2.43.0

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[Jens Axboe]

On Mon, 28 Apr 2025 22:36:26 +0800, Lizhi Xu wrote:
> Some file systems do not support read_iter/write_iter, such as selinuxfs
> in this issue.
> So before calling them, first confirm that the interface is supported and
> then call it.
> 
> It is releavant in that vfs_iter_read/write have the check, and removal
> of their used caused szybot to be able to hit this issue.
> 
> [...]

Applied, thanks!

[1/1] loop: Add sanity check for read/write_iter
      commit: f5c84eff634ba003326aa034c414e2a9dcb7c6a7

Best regards,
-- 
Jens Axboe

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[Christian Hesse]

[-- Attachment #1: Type: text/plain, Size: 2139 bytes --]

Lizhi Xu <lizhi.xu@windriver.com> on Mon, 2025/04/28 22:36:
> Some file systems do not support read_iter/write_iter, such as selinuxfs
> in this issue.
> So before calling them, first confirm that the interface is supported and
> then call it.
> 
> It is releavant in that vfs_iter_read/write have the check, and removal
> of their used caused szybot to be able to hit this issue.
> 
> Fixes: f2fed441c69b ("loop: stop using vfs_iter__{read,write} for buffered
> I/O") Reported-by: syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> V1 -> V2: move check to loop_configure and loop_change_fd
> V2 -> V3: using helper for this check
> V3 -> V4: remove input parameters change and mode
> V4 -> V5: remove braces around !file->f_op->write_iter

This introduced a regression for Arch Linux, breaking boot media generated
with archiso [0]. More specifically it's this call of losetup [1].

There's a squashfs inside iso9660. Mounting the iso9660 filesystem works
fine, but losetup complains when setting up:

$ losetup --find --show --read-only -- /run/archiso/bootmnt/arch/x86_64/airootfs.sfs
losetup: /run/archiso/bootmnt/arch/x86_64/airootfs.sfs: failed to set up loop device: Invalid argument

This has been bisected to commit d278164832618bf2775c6a89e6434e2633de1eed in
mainline (and 9bd3feb324fce2e93e09d0a5b00887e81d337a8c for linux-6.14.y,
184b147b9f7f07577567a80fcc9314f2bd0b0b00 for linux-6.12.y). Thanks to
Christian Heusel for his work on this.

As the call tries to setup in read-only mode the check for
(file->f_op->read_iter) fails here, returning the -EINVAL we see.

Reported-by: Christian Hesse <mail@eworm.de>
Bisected-by: Christian Heusel <christian@heusel.eu>

[0] https://gitlab.archlinux.org/archlinux/mkinitcpio/mkinitcpio-archiso
[1] https://gitlab.archlinux.org/archlinux/mkinitcpio/mkinitcpio-archiso/-/blob/master/hooks/archiso?ref_type=heads#L88
-- 
Mit freundlichen Gruessen
Christian Hesse

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[Lizhi Xu]

On Mon, 19 May 2025 17:56:40 +0200, Christian Hesse wrote:
> $ losetup --find --show --read-only -- /run/archiso/bootmnt/arch/x86_64/airootfs.sfs
> losetup: /run/archiso/bootmnt/arch/x86_64/airootfs.sfs: failed to set up loop device: Invalid argument
I tried to reproduce the problem you mentioned using the kernel containing
"commit:f5c84eff", but failed to reproduce it.
The complete reproduction steps are as follows:

sudo apt install squashfs-tools debootstrap
sudo debootstrap --arch=amd64 focal rootfs http://archive.ubuntu.com/ubuntu/
sudo mksquashfs rootfs rootfs.sfs -comp xz -e boot
root@intel-x86-64:~# losetup --find --show --read-only -- rootfs.sfs
[   79.676267][ T9551] loop0: detected capacity change from 0 to 272400
/dev/loop0
root@intel-x86-64:~# uname -a
Linux intel-x86-64 6.15.0-rc7 #108 SMP PREEMPT_DYNAMIC Mon May 19 09:20:25 CST 2025 x86_64 x86_64 x86_64 GNU/Linux
root@intel-x86-64:~# df -Th
Filesystem     Type      Size  Used Avail Use% Mounted on
/dev/root      ext4      3.8G  738M  2.9G  21% /
devtmpfs       devtmpfs  1.5G     0  1.5G   0% /dev
tmpfs          tmpfs     1.5G     0  1.5G   0% /dev/shm
tmpfs          tmpfs     585M   13M  572M   3% /run
tmpfs          tmpfs     4.0M     0  4.0M   0% /sys/fs/cgroup
tmpfs          tmpfs     1.5G     0  1.5G   0% /tmp
tmpfs          tmpfs     1.5G  904K  1.5G   1% /var/volatile
tmpfs          tmpfs     293M     0  293M   0% /run/user/0
root@intel-x86-64:~# ls -lath /dev/loop0
brw-rw---- 1 root disk 7, 0 May 20 02:43 /dev/loop0
root@intel-x86-64:~# mkdir sfs
root@intel-x86-64:~# mount /dev/loop0 sfs
mount: /root/sfs: WARNING: source write-protected, mounted read-only.
root@intel-x86-64:~# df -Th
Filesystem     Type      Size  Used Avail Use% Mounted on
/dev/root      ext4      3.8G  738M  2.9G  21% /
devtmpfs       devtmpfs  1.5G     0  1.5G   0% /dev
tmpfs          tmpfs     1.5G     0  1.5G   0% /dev/shm
tmpfs          tmpfs     585M   13M  572M   3% /run
tmpfs          tmpfs     4.0M     0  4.0M   0% /sys/fs/cgroup
tmpfs          tmpfs     1.5G     0  1.5G   0% /tmp
tmpfs          tmpfs     1.5G  904K  1.5G   1% /var/volatile
tmpfs          tmpfs     293M     0  293M   0% /run/user/0
/dev/loop0     squashfs  134M  134M     0 100% /root/sfs
root@intel-x86-64:~# ls -alt sfs
total 3
drwx------ 21 root root 3072 May 20 02:49 ..
drwxr-xr-x 16 root root  284 May 20 02:20 .
drwxrwxrwt  2 root root    3 May 20 02:20 tmp
drwxr-xr-x 59 root root 2073 May 20 02:20 etc
drwxr-xr-x  8 root root  124 May 20 02:20 run
drwxr-xr-x  2 root root    3 May 20 02:19 media
drwxr-xr-x  2 root root    3 May 20 02:19 mnt
drwxr-xr-x  2 root root    3 May 20 02:19 opt
drwx------  2 root root   46 May 20 02:19 root
drwxr-xr-x  2 root root    3 May 20 02:19 srv
drwxr-xr-x 13 root root  178 May 20 02:19 usr
drwxr-xr-x 11 root root  172 May 20 02:19 var
drwxr-xr-x  4 root root  191 May 20 02:19 dev
lrwxrwxrwx  1 root root    7 May 20 02:19 bin -> usr/bin
lrwxrwxrwx  1 root root    7 May 20 02:19 lib -> usr/lib
lrwxrwxrwx  1 root root    9 May 20 02:19 lib32 -> usr/lib32
lrwxrwxrwx  1 root root    9 May 20 02:19 lib64 -> usr/lib64
lrwxrwxrwx  1 root root   10 May 20 02:19 libx32 -> usr/libx32
lrwxrwxrwx  1 root root    8 May 20 02:19 sbin -> usr/sbin
drwxr-xr-x  2 root root    3 Apr 15  2020 home
drwxr-xr-x  2 root root    3 Apr 15  2020 proc
drwxr-xr-x  2 root root    3 Apr 15  2020 sys

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[Christian Hesse]

[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]

Lizhi Xu <lizhi.xu@windriver.com> on Tue, 2025/05/20 11:00:
> On Mon, 19 May 2025 17:56:40 +0200, Christian Hesse wrote:
> > $ losetup --find --show --read-only --
> > /run/archiso/bootmnt/arch/x86_64/airootfs.sfs losetup:
> > /run/archiso/bootmnt/arch/x86_64/airootfs.sfs: failed to set up loop
> > device: Invalid argument
>
> I tried to reproduce the problem you mentioned using the kernel containing
> "commit:f5c84eff", but failed to reproduce it.
> The complete reproduction steps are as follows:
> 
> sudo apt install squashfs-tools debootstrap
> sudo debootstrap --arch=amd64 focal rootfs http://archive.ubuntu.com/ubuntu/
> sudo mksquashfs rootfs rootfs.sfs -comp xz -e boot
> [...]

That's the wrong end of the stack. After all squashfs is not directly
involved here (that was just an etxra info on why we have a loopback file
inside iso9660).

The issue is setting up the loopback file inside a mounted iso9660 filesystem.
Take these steps for easy reproduction:

root@leda ~ # mkdir iso.d 
root@leda ~ # truncate -s 10m iso.d/loopback.img
root@leda ~ # mkisofs -o iso.iso iso.d/
Setting input-charset to 'UTF-8' from locale.
 94,75% done, estimate finish Tue May 20 07:34:52 2025
Total translation table size: 0
Total rockridge attributes bytes: 0
Total directory bytes: 0
Path table size(bytes): 10
Max brk space used 0
5294 extents written (10 MB)
root@leda ~ # mount -o loop iso.iso /mnt/tmp 
mount: /mnt/tmp: WARNING: source write-protected, mounted read-only.
root@leda ~ # losetup --find --show --read-only -- /mnt/tmp/loopback.img 
losetup: /mnt/tmp/loopback.img: failed to set up loop device: Invalid argument

Hope that helps, let me know if you need more assistance.
-- 
Best regards,
Chris

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

回复: [PATCH V5] loop: Add sanity check for read/write_iter

[Xu, Lizhi]

I figured out your steps to reproduce, and yes, this problem will occur if you do losetup with a file in a filesystem that does not support read_iter, which is what this patch does.

________________________________________
发件人: Christian Hesse
已发送: 2025 年 5 月 20 日 星期二 13:39
收件人: Xu, Lizhi
抄送: axboe@kernel.dk; christian@heusel.eu; hch@infradead.org; linux-block@vger.kernel.org; linux-kernel@vger.kernel.org; ming.lei@redhat.com; syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com; syzkaller-bugs@googlegroups.com
主题: Re: [PATCH V5] loop: Add sanity check for read/write_iter


Lizhi Xu <lizhi.xu@windriver.com> on Tue, 2025/05/20 11:00:
> On Mon, 19 May 2025 17:56:40 +0200, Christian Hesse wrote:
> > $ losetup --find --show --read-only --
> > /run/archiso/bootmnt/arch/x86_64/airootfs.sfs losetup:
> > /run/archiso/bootmnt/arch/x86_64/airootfs.sfs: failed to set up loop
> > device: Invalid argument
>
> I tried to reproduce the problem you mentioned using the kernel containing
> "commit:f5c84eff", but failed to reproduce it.
> The complete reproduction steps are as follows:
> 
> sudo apt install squashfs-tools debootstrap
> sudo debootstrap --arch=amd64 focal rootfs http://archive.ubuntu.com/ubuntu/
> sudo mksquashfs rootfs rootfs.sfs -comp xz -e boot
> [...]

That's the wrong end of the stack. After all squashfs is not directly
involved here (that was just an etxra info on why we have a loopback file
inside iso9660).

The issue is setting up the loopback file inside a mounted iso9660 filesystem.
Take these steps for easy reproduction:

root@leda ~ # mkdir iso.d 
root@leda ~ # truncate -s 10m iso.d/loopback.img
root@leda ~ # mkisofs -o iso.iso iso.d/
Setting input-charset to 'UTF-8' from locale.
 94,75% done, estimate finish Tue May 20 07:34:52 2025
Total translation table size: 0
Total rockridge attributes bytes: 0
Total directory bytes: 0
Path table size(bytes): 10
Max brk space used 0
5294 extents written (10 MB)
root@leda ~ # mount -o loop iso.iso /mnt/tmp 
mount: /mnt/tmp: WARNING: source write-protected, mounted read-only.
root@leda ~ # losetup --find --show --read-only -- /mnt/tmp/loopback.img 
losetup: /mnt/tmp/loopback.img: failed to set up loop device: Invalid argument

Hope that helps, let me know if you need more assistance.
-- 
Best regards,
Chris

Re: 回复: [PATCH V5] loop: Add sanity check for read/write_iter

[Christian Hesse]

[-- Attachment #1: Type: text/plain, Size: 375 bytes --]

"Xu, Lizhi" <Lizhi.Xu@windriver.com> on Tue, 2025/05/20 06:29:
> I figured out your steps to reproduce, and yes, this problem will occur if
> you do losetup with a file in a filesystem that does not support read_iter,
> which is what this patch does.

So is this expected behavior now?

It worked before... How to recover for our use case?
-- 
Best regards,
Chris

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: 回复: [PATCH V5] loop: Add sanity check for read/write_iter

[hch]

On Tue, May 20, 2025 at 06:29:48AM +0000, Xu, Lizhi wrote:
> I figured out your steps to reproduce, and yes, this problem will occur if you do losetup with a file in a filesystem that does not support read_iter, which is what this patch does.

isofs does support read_iter, without that it would not have worked
before either. That is not the problem.  It must be related to
the FMODE_WRITE check - i.e. we have a writable FD here, but a file
system that does not actually supports writes.  Which honestly feels
weird, but we'll have to figure it out to unbreak these setups.

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[Xu, Lizhi]

It was designed that way. But for now, if such files from a filesystem
that does not support read_iter are mounted to the loop device without
performing read operations, it may be safe.

It would be best if Christoph Hellwig could give some comments.

BR,
Lizhi

回复: 回复: [PATCH V5] loop: Add sanity check for read/write_iter

[Xu, Lizhi]

No, isofs does not support read_iter.

________________________________________
发件人: hch@infradead.org <hch@infradead.org>
发送时间: 2025年5月20日 14:46
收件人: Xu, Lizhi
抄送: Christian Hesse; axboe@kernel.dk; christian@heusel.eu; hch@infradead.org; linux-block@vger.kernel.org; linux-kernel@vger.kernel.org; ming.lei@redhat.com; syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com; syzkaller-bugs@googlegroups.com
主题: Re: 回复: [PATCH V5] loop: Add sanity check for read/write_iter

CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.

On Tue, May 20, 2025 at 06:29:48AM +0000, Xu, Lizhi wrote:
> I figured out your steps to reproduce, and yes, this problem will occur if you do losetup with a file in a filesystem that does not support read_iter, which is what this patch does.

isofs does support read_iter, without that it would not have worked
before either. That is not the problem.  It must be related to
the FMODE_WRITE check - i.e. we have a writable FD here, but a file
system that does not actually supports writes.  Which honestly feels
weird, but we'll have to figure it out to unbreak these setups.

Re: 回复: 回复: [PATCH V5] loop: Add sanity check for read/write_iter

[hch]

On Tue, May 20, 2025 at 06:59:14AM +0000, Xu, Lizhi wrote:
> No, isofs does not support read_iter.

Of course it does, check again.

回复: 回复: 回复: [PATCH V5] loop: Add sanity check for read/write_iter

[Xu, Lizhi]

No, isofs does not support read_iter.

________________________________________
发件人: hch@infradead.org <hch@infradead.org>
发送时间: 2025年5月20日 19:28
收件人: Xu, Lizhi
抄送: hch@infradead.org; Christian Hesse; axboe@kernel.dk; christian@heusel.eu; linux-block@vger.kernel.org; linux-kernel@vger.kernel.org; ming.lei@redhat.com; syzbot+6af973a3b8dfd2faefdc@syzkaller.appspotmail.com; syzkaller-bugs@googlegroups.com
主题: Re: 回复: 回复: [PATCH V5] loop: Add sanity check for read/write_iter

CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.

On Tue, May 20, 2025 at 06:59:14AM +0000, Xu, Lizhi wrote:
> No, isofs does not support read_iter.

Of course it does, check again.

Re: 回复: 回复: 回复: [PATCH V5] loop: Add sanity check for read/write_iter

[hch]

On Tue, May 20, 2025 at 11:39:48AM +0000, Xu, Lizhi wrote:
> No, isofs does not support read_iter.

isofs does use generic_file_read_iter as read_iter method through
generic_ro_fops.  Why do you keep insisting on a wrong answer instead
of simply checking what read* method is used by isofs?

回复: 回复: 回复: 回复: [PATCH V5] loop: Add sanity check for read/write_iter

[Xu, Lizhi]

generic_ro_fops, Oh, got it, I didn't find it.

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[Xu, Lizhi]

I added debugging code in loop_check_backing_file(), read_iter is NULL,
and I searched for read_iter in directly, so it says isofs does not support it.
I see isofs_read_inode is set in isofs_read_inode(), I will further investigate
why it is not set.

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[Xu, Lizhi]

> isofs does support read_iter, without that it would not have worked
> before either. That is not the problem.  It must be related to
> the FMODE_WRITE check - i.e. we have a writable FD here, but a file
> system that does not actually supports writes.  Which honestly feels
> weird, but we'll have to figure it out to unbreak these setups.
If it is a directory, isofs_dir_operations is used. In this case,
isofs does not support read_iter (I used a directory when testing).
If it is a regular file, generic_ro_fops is used. In this case,
isofs supports read_iter. When a regular file has a writable attribute,
the problem will recur because isofs does not support write_iter.

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[Christian Hesse]

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]

"Xu, Lizhi" <Lizhi.Xu@windriver.com> on Tue, 2025/05/20 12:27:
> If it is a regular file, generic_ro_fops is used. In this case,
> isofs supports read_iter. When a regular file has a writable attribute,

Just tested with an iso file where writable flag from loopback file inside
was explicitly removed. No change.

> the problem will recur because isofs does not support write_iter.

We have two indications here that setup should happen in read-only mode:

* The underlaying filesystem is read-only
* `losetup` is called with switch `--read-only`

I would expect both to make this happy.
-- 
Best regards,
Chris

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[hch]

On Tue, May 20, 2025 at 12:27:44PM +0000, Xu, Lizhi wrote:
> isofs does not support read_iter (I used a directory when testing).
> If it is a regular file, generic_ro_fops is used. In this case,
> isofs supports read_iter. When a regular file has a writable attribute,
> the problem will recur because isofs does not support write_iter.

All Linux filesystems do (or at least should) point read on directories
to generic_read_dir which returns -EISDIR as reading from directories
is not supported.

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[hch]

On Tue, May 20, 2025 at 02:46:22PM +0200, Christian Hesse wrote:
> "Xu, Lizhi" <Lizhi.Xu@windriver.com> on Tue, 2025/05/20 12:27:
> > If it is a regular file, generic_ro_fops is used. In this case,
> > isofs supports read_iter. When a regular file has a writable attribute,
> 
> Just tested with an iso file where writable flag from loopback file inside
> was explicitly removed. No change.
> 
> > the problem will recur because isofs does not support write_iter.
> 
> We have two indications here that setup should happen in read-only mode:
> 
> * The underlaying filesystem is read-only
> * `losetup` is called with switch `--read-only`
> 
> I would expect both to make this happy.

Can you test this patch?

We historically allow a writable fd on block devices even when they
are read-only.  I suspect your use case is doing that and the new
check for write_iter is interfering with that:


diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index b8ba7de08753..e2b1f377f585 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -979,9 +979,6 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 	if (!file)
 		return -EBADF;
 
-	if ((mode & BLK_OPEN_WRITE) && !file->f_op->write_iter)
-		return -EINVAL;
-
 	error = loop_check_backing_file(file);
 	if (error)
 		return error;

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[Xu, Lizhi]

> We historically allow a writable fd on block devices even when they
> are read-only.  I suspect your use case is doing that and the new
> check for write_iter is interfering with that:
After deleting the "if ((mode & BLK_OPEN_WRITE) && !file->f_op->write_iter)",
everything should be normal.

Re: [PATCH V5] loop: Add sanity check for read/write_iter

[christian]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

On 25/05/20 05:49AM, hch@infradead.org wrote:
> On Tue, May 20, 2025 at 02:46:22PM +0200, Christian Hesse wrote:
> > "Xu, Lizhi" <Lizhi.Xu@windriver.com> on Tue, 2025/05/20 12:27:
> > > If it is a regular file, generic_ro_fops is used. In this case,
> > > isofs supports read_iter. When a regular file has a writable attribute,
> > 
> > Just tested with an iso file where writable flag from loopback file inside
> > was explicitly removed. No change.
> > 
> > > the problem will recur because isofs does not support write_iter.
> > 
> > We have two indications here that setup should happen in read-only mode:
> > 
> > * The underlaying filesystem is read-only
> > * `losetup` is called with switch `--read-only`
> > 
> > I would expect both to make this happy.
> 
> Can you test this patch?
> 
> We historically allow a writable fd on block devices even when they
> are read-only.  I suspect your use case is doing that and the new
> check for write_iter is interfering with that:

I have tested the patch and can confirm that it fixes the usecase as
represented by the reproducer that I have used to bisect the bug.

If you turn this into an actual patch you can add my

Tested-by: Christian Heusel <christian@heusel.eu>

if you want :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]