[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+6af973...@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 syzk...@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

[Lizhi Xu]

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,

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
BUG: unable to handle kernel NULL pointer dereference in filemap_read_folio

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: 2 UID: 0 PID: 46 Comm: kworker/u32:2 Not tainted 6.15.0-rc3-syzkaller-g02ddfb981de8-dirty #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:ffffc90000a3f5a0 EFLAGS: 00010293
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff81f2b3ae
RDX: ffff88801f2bc880 RSI: ffffea0000e2d740 RDI: ffff88801fc48e00
RBP: ffffea0000e2d740 R08: 0000000000000007 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000002be0 R12: 1ffff92000147eb5
R13: ffff88801fc48e00 R14: 0000000000000000 R15: dffffc0000000000
FS: 0000000000000000(0000) GS:ffff8880d6bb2000(0000) knlGS:0000000000000000

CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033

CR2: ffffffffffffffd6 CR3: 0000000012f76000 CR4: 0000000000352ef0

DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>

filemap_read_folio+0xc5/0x2a0 mm/filemap.c:2401
filemap_create_folio mm/filemap.c:2536 [inline]
filemap_get_pages+0xf39/0x1c20 mm/filemap.c:2597
filemap_read+0x3d2/0xe90 mm/filemap.c:2702
generic_file_read_iter+0x344/0x450 mm/filemap.c:2894

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:ffffc90000a3f5a0 EFLAGS: 00010293
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff81f2b3ae
RDX: ffff88801f2bc880 RSI: ffffea0000e2d740 RDI: ffff88801fc48e00
RBP: ffffea0000e2d740 R08: 0000000000000007 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000002be0 R12: 1ffff92000147eb5
R13: ffff88801fc48e00 R14: 0000000000000000 R15: dffffc0000000000
FS: 0000000000000000(0000) GS:ffff8880d6bb2000(0000) knlGS:0000000000000000

CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033

CR2: ffffffffffffffd6 CR3: 0000000012f76000 CR4: 0000000000352ef0

DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400

Tested on:

commit: 02ddfb98 Merge tag 'scsi-fixes' of git://git.kernel.or..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=16aefd9b980000
kernel config: https://syzkaller.appspot.com/x/.config?x=efa83f9a6dd67d67

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

patch: https://syzkaller.appspot.com/x/patch.diff?x=110efd9b980000

[Lizhi Xu]

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,

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-by: syzbot+6af973...@syzkaller.appspotmail.com
Tested-by: syzbot+6af973...@syzkaller.appspotmail.com

Tested on:

commit: 02ddfb98 Merge tag 'scsi-fixes' of git://git.kernel.or..
git tree: upstream

console output: https://syzkaller.appspot.com/x/log.txt?x=14a92574580000

kernel config: https://syzkaller.appspot.com/x/.config?x=efa83f9a6dd67d67
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

patch: https://syzkaller.appspot.com/x/patch.diff?x=1411a1b3980000

Note: testing is done by a robot and is best-effort only.

[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+6af973...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
Signed-off-by: Lizhi Xu <lizh...@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

[Zhu Yanjun]

"else if" is better?

Zhu Yanjun

[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.

[Ming Lei]

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

Thanks,

[Lizhi Xu]

Yes, you are right, I will test and send V2 patch.

BR,
Lizhi

[Lizhi Xu]

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(). */

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-by: syzbot+6af973...@syzkaller.appspotmail.com
Tested-by: syzbot+6af973...@syzkaller.appspotmail.com

Tested on:

commit: 02ddfb98 Merge tag 'scsi-fixes' of git://git.kernel.or..
git tree: upstream

console output: https://syzkaller.appspot.com/x/log.txt?x=15b3fd9b980000

kernel config: https://syzkaller.appspot.com/x/.config?x=efa83f9a6dd67d67
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

patch: https://syzkaller.appspot.com/x/patch.diff?x=13467fcf980000

[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+6af973...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
Signed-off-by: Lizhi Xu <lizh...@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

[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?

[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.

[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+6af973...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
Signed-off-by: Lizhi Xu <lizh...@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;

[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.

[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.

[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.

[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.

[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.

[Christoph Hellwig]

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.

[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+6af973...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
Signed-off-by: Lizhi Xu <lizh...@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);

[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 <h...@lst.de>

[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+6af973...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
Signed-off-by: Lizhi Xu <lizh...@windriver.com>

Reviewed-by: Christoph Hellwig <h...@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)

[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

[Christian Hesse]

Lizhi Xu <lizh...@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+6af973...@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6af973a3b8dfd2faefdc
> Signed-off-by: Lizhi Xu <lizh...@windriver.com>
> Reviewed-by: Christoph Hellwig <h...@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 <ma...@eworm.de>
Bisected-by: Christian Heusel <chri...@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

[Xu, Lizhi]

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

---

发件人: Christian Hesse
已发送: 2025 年 5 月 19 日 星期一 23:56
收件人: Xu, Lizhi
抄送: h...@infradead.org; ax...@kernel.dk; linux...@vger.kernel.org; linux-...@vger.kernel.org; ming...@redhat.com; syzbot+6af973...@syzkaller.appspotmail.com; syzkall...@googlegroups.com; Christian Heusel; Christian Hesse
主题: 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

[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
抄送: ax...@kernel.dk; chri...@heusel.eu; h...@infradead.org; linux...@vger.kernel.org; linux-...@vger.kernel.org; ming...@redhat.com; syzbot+6af973...@syzkaller.appspotmail.com; syzkall...@googlegroups.com

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

Lizhi Xu <lizh...@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

[h...@infradead.org]

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.

[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

[Xu, Lizhi]

No, isofs does not support read_iter.

________________________________________
发件人: h...@infradead.org <h...@infradead.org>
发送时间: 2025年5月20日 14:46
收件人: Xu, Lizhi
抄送: Christian Hesse; ax...@kernel.dk; chri...@heusel.eu; h...@infradead.org; linux...@vger.kernel.org; linux-...@vger.kernel.org; ming...@redhat.com; syzbot+6af973...@syzkaller.appspotmail.com; syzkall...@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.

[Christian Hesse]

Lizhi Xu <lizh...@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

[Christian Hesse]

"Xu, Lizhi" <Lizh...@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

[h...@infradead.org]

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.

[Xu, Lizhi]

No, isofs does not support read_iter.

________________________________________
发件人: h...@infradead.org <h...@infradead.org>
发送时间: 2025年5月20日 19:28
收件人: Xu, Lizhi
抄送: h...@infradead.org; Christian Hesse; ax...@kernel.dk; chri...@heusel.eu; linux...@vger.kernel.org; linux-...@vger.kernel.org; ming...@redhat.com; syzbot+6af973...@syzkaller.appspotmail.com; syzkall...@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.

[h...@infradead.org]

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?

[Xu, Lizhi]

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

[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.

[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.

[h...@infradead.org]

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.

[h...@infradead.org]

On Tue, May 20, 2025 at 02:46:22PM +0200, Christian Hesse wrote:
> "Xu, Lizhi" <Lizh...@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;

[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.

[Christian Hesse]

"Xu, Lizhi" <Lizh...@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

[chri...@heusel.eu]

On 25/05/20 05:49AM, h...@infradead.org wrote:
> On Tue, May 20, 2025 at 02:46:22PM +0200, Christian Hesse wrote:
> > "Xu, Lizhi" <Lizh...@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 <chri...@heusel.eu>

if you want :)