Re: [PATCH V11 7/9] famfs_fuse: fail I/O on invalid or errored daxdevs

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Returning -EHWPOISON from iomap_begin leaks the internal error code to read() and write() syscalls.
- [Medium] Permanent invalidation of the file via `meta->error = true` masks hardware poison errors on subsequent page faults.
- [Low] Data race on shared boolean flags without READ_ONCE() and WRITE_ONCE().
--

--- Patch [7]: [PATCH V11 7/9] famfs_fuse: fail I/O on invalid or errored daxdevs ---
commit baadfa284b86538249581ed04638cf37b1935608
Author: John Groves <[email protected]>

famfs_fuse: fail I/O on invalid or errored daxdevs

This commit gates the iomap resolution path on the state of the daxdev
backing each referenced extent. If the daxdev is invalid, has a dax_err,
or reports a memory error, famfs_fileofs_to_daxofs() marks the file with
an error and prevents further access.

> diff --git a/fs/fuse/famfs.c b/fs/fuse/famfs.c
> index ac56317944d93..8d13282e8949f 100644
> --- a/fs/fuse/famfs.c
> +++ b/fs/fuse/famfs.c
[ ... ]
> @@ -589,6 +589,26 @@ famfs_file_init_dax(
>  
>  static int famfs_file_bad(struct inode *inode);
>  
> +static int famfs_dax_err(struct famfs_daxdev *dd)
> +{
> +	if (!dd->valid) {

[Severity: Low]
Are these shared boolean flags accessed concurrently without protection?

Hardware memory poison can asynchronously modify the device state flags
(dd->valid, dd->dax_err, dd->error). Should these be protected with
READ_ONCE() to avoid data races and KCSAN warnings?

> +		pr_err("%s: daxdev=%s invalid\n",
> +		       __func__, dd->name);
> +		return -EIO;
> +	}
> +	if (dd->dax_err) {
> +		pr_err("%s: daxdev=%s dax_err\n",
> +		       __func__, dd->name);
> +		return -EIO;
> +	}
> +	if (dd->error) {
> +		pr_err("%s: daxdev=%s memory error\n",
> +		       __func__, dd->name);
> +		return -EHWPOISON;

[Severity: Medium]
Is there a risk of leaking the internal -EHWPOISON error code to read() or
write() syscalls here?

If a userspace process calls read() or write() on a file backed by a
poisoned daxdev, famfs_fuse_read_iter() calls dax_iomap_rw(), which calls
iomap_iter() and subsequently famfs_fuse_iomap_begin().

Returning -EHWPOISON here bypasses the generic DAX error translation
(dax_mem2blk_err) and is returned directly to the syscall. This gives
userspace an unfamiliar error code (-1 with errno = 133) instead of
standard POSIX error codes like EIO.

> +	}
> +	return 0;
> +}
> +
>  /**
>   * famfs_fileofs_to_daxofs() - Resolve (file, offset, len) to (daxdev, offset, len)
>   *
[ ... ]
> @@ -670,6 +691,13 @@ famfs_fileofs_to_daxofs(struct inode *inode, struct iomap *iomap,
>  
>  		dd = &fc->dax_devlist->devlist[daxdev_idx];
>  
> +		rc = famfs_dax_err(dd);
> +		if (rc) {
> +			/* Shut down access to this file */
> +			meta->error = true;

[Severity: Medium]
Does permanently flagging the file as errored mask hardware poison errors on
subsequent page faults?

When famfs_fileofs_to_daxofs() first encounters a memory error, it returns
-EHWPOISON but permanently sets meta->error = true.

On any subsequent fault to the file, famfs_file_bad() evaluates to true,
causing famfs_fileofs_to_daxofs() to branch to err_out which unconditionally
returns -EIO instead of -EHWPOISON.

This causes subsequent page faults on poisoned memory to deliver a generic
SIGBUS (BUS_ADRERR) instead of a hardware memory error SIGBUS
(BUS_MCEERR_AR).

[Severity: Low]
Also, similar to the read side in famfs_dax_err(), should this concurrent
write to meta->error use WRITE_ONCE() to prevent data races?

> +			return rc;
> +		}
> +
>  		iomap->addr    = dax_ext_offset + local_offset;
>  		iomap->offset  = file_offset;
>  		iomap->length  = min_t(loff_t, len, ext_len_remainder);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019f7d9fbe81-6cb16662-2522-47ea-a152-fab0ee3d9b35-000000@email.amazonses.com?part=7