kernfs, sysfs: refuse splice on /sys/power/resume
Subsystems (6)
v3 cover kernfs, sysfs: refuse splice on /sys/power/resume
This series refuses splice on writable opens of /sys/power/resume and gives those opens separate lockdep classes, as Tejun suggested in the v1 discussion.
The reported cycle is assembled from two operations. Splice into an ordinary writable kernfs file records pipe->mutex before of->mutex. A normal write to /sys/power/resume records of->mutex before the autofs pipe locks when resume_store() resolves a relative path.
Patch 1 adds SYSFS_NO_SPLICE and kernfs_ops->no_splice. Writable opens of a marked file refuse both splice directions. Their of->mutex and seq_file lock use separate classes. Read-only opens retain splice support.
Patch 2 marks /sys/power/resume. Normal reads and writes stay unchanged, including a write which resolves a relative path through autofs. Rafael acked it to go together with patch 1.
Other stores also look up paths. They remain outside this minimal series and can be marked in follow-up patches.
Tested on v7.4-rc1 in a QEMU guest with the syzbot kernel config. Without the series, the C and syz reproducers trigger the reported lockdep cycle, and a splice from the autofs daemon pipe into /sys/power/resume deadlocks. With the series, neither reproducer triggers a lockdep report although autofs receives the same requests, and that splice fails with EINVAL. Plain reads and writes of /sys/power/resume, read-only splice and splice on other sysfs and cgroup files work as before.
v3:
- Rename SYSFS_LOOKUP and ->may_lookup to SYSFS_NO_SPLICE and ->no_splice (Tejun).
- Drop the mmap, prealloc and binary attribute checks (Tejun).
- Add Rafael's Acked-by to patch 2.
v2:
- Replace the v1 locking changes with per-attribute splice control.
- Preserve splice on read-only opens.
- Split both the open-file mutex and seq_file lock classes.
- Reject incompatible mmap and preallocated-buffer combinations.
- Reject SYSFS_LOOKUP on binary attributes.
- Rebase onto v7.4-rc1.
v2: https://lore.kernel.org/r/[email protected]
v1: https://lore.kernel.org/r/[email protected]
Krystian Kaniewski (2):
kernfs, sysfs: add SYSFS_NO_SPLICE to refuse splice on writable opens
PM: hibernate: refuse splice on /sys/power/resume
fs/kernfs/file.c | 46 +++++++++++++++++++++++++++++++++++++--- fs/sysfs/file.c | 18 ++++++++++++++++ fs/sysfs/group.c | 4 ++-- include/linux/kernfs.h | 9 ++++++++ include/linux/sysfs.h | 22 +++++++++++++------ kernel/power/hibernate.c | 3 ++- 6 files changed, 90 insertions(+), 12 deletions(-)
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
Signature · 1 line
2.53.0
PATCH v3 1/2 kernfs, sysfs: add SYSFS_NO_SPLICE to refuse splice on writable opens
Some sysfs stores look up a path written by the user, for example /sys/power/resume through lookup_bdev(). When the path reaches an autofs mount, autofs writes a mount request to its daemon pipe while kernfs holds of->mutex. splice() enters kernfs files with a pipe mutex held, and writable opens of all kernfs files without mmap share one lockdep class for of->mutex, so lockdep sees
&pipe->mutex --> &of->mutex --> &sbi->pipe_mutex --> &pipe->mutex
A splice into any writable kernfs file can add the first edge. A splice from the autofs daemon pipe into a file whose store looks up the path is a real deadlock, because autofs then waits for the mutex of the pipe that the splice holds.
Add kernfs_ops->no_splice and SYSFS_NO_SPLICE for such files. Writable opens of a marked file refuse both splice directions with -EINVAL before the file callback runs, and their of->mutex and seq_file lock get lockdep classes of their own. The seq_file lock needs its own class as well, because a read through such an open takes of->mutex under it, while a splice from any other seq_file takes the shared seq_file class under a pipe mutex. Read-only opens keep splice support because they cannot reach ->write().
---
v3:
- Rename SYSFS_LOOKUP and ->may_lookup to SYSFS_NO_SPLICE and ->no_splice (Tejun).
- Drop the mmap, prealloc and binary attribute checks, document the limits in comments instead (Tejun).
v2:
- Replace the v1 locking changes with per-attribute splice control.
- Preserve splice on read-only opens.
- Split both the open-file mutex and seq_file lock classes.
- Reject incompatible mmap and preallocated-buffer combinations.
- Reject SYSFS_LOOKUP on binary attributes.
- Rebase onto v7.4-rc1.
fs/kernfs/file.c | 46 +++++++++++++++++++++++++++++++++++++++--- fs/sysfs/file.c | 18 +++++++++++++++++ fs/sysfs/group.c | 4 ++-- include/linux/kernfs.h | 9 +++++++++ include/linux/sysfs.h | 22 ++++++++++++++------ 5 files changed, 88 insertions(+), 11 deletions(-)
fs/kernfs/file.c +43 -3
@@ -649,6 +649,12 @@ static int kernfs_fop_open(struct inode *inode, struct file *file)if (!of) goto err_out; +/* +* Splice callbacks need this before the iter paths acquire an active +* reference, so cache it while @kn is active. +*/ + of->no_splice = (file->f_mode &FMODE_WRITE ) && ops->no_splice; +/* * The following is done to give a different lockdep key to * @of->mutex for files which implement mmap. This is a rather
@@ -666,11 +672,17 @@ static int kernfs_fop_open(struct inode *inode, struct file *file)* lookup helpers for arbitrary paths and readonly files can be read by * overlayfs from vfs helpers when sysfs is a lower layer of overalyfs. * -* All three cases look the same. They're supposed to +* Writable opens of ->no_splice files get a fourth key. Their +* ->write() may wait for userspace through a pipe, so splice, which +* takes @of->mutex under a pipe mutex, is refused for them. +* +* All four cases look the same. They're supposed to * look that way and give @of->mutex different static lockdep keys. */ if (has_mmap)mutex_init (&of->mutex); +else if (of->no_splice) +mutex_init (&of->mutex);else if (file->f_mode &FMODE_WRITE )mutex_init (&of->mutex);else
@@ -717,6 +729,16 @@ static int kernfs_fop_open(struct inode *inode, struct file *file) of->seq_file = file->private_data; of->seq_file->private = of; +/* +* seq_read_iter() takes @of->mutex under the seq_file lock, whose key +* is shared with seq_files that can be spliced. Separate it too. +*/ +if (of->no_splice) { +static struct lock_class_key seq_no_splice_key; + +lockdep_set_class (&of->seq_file->lock, &seq_no_splice_key); + } +/* seq_file clears PWRITE unconditionally, restore it if WRITE */ if (file->f_mode &FMODE_WRITE ) file->f_mode |=FMODE_PWRITE ;
@@ -889,6 +911,24 @@ static __poll_t kernfs_fop_poll(struct file *filp, poll_table *wait)return ret; } +static ssize_t kernfs_fop_splice_read (struct file *in,loff_t *ppos, +struct pipe_inode_info *pipe,size_t len, +unsigned int flags) +{ +if (kernfs_of (in)->no_splice) +return -EINVAL ; +return copy_splice_read (in, ppos, pipe, len, flags); +} + +static ssize_t kernfs_fop_splice_write (struct pipe_inode_info *pipe, +struct file *out,loff_t *ppos, +size_t len,unsigned int flags) +{ +if (kernfs_of (out)->no_splice) +return -EINVAL ; +return iter_file_splice_write (pipe, out, ppos, len, flags); +} +static loff_t kernfs_fop_llseek (struct file *file,loff_t offset,int whence) {struct kernfs_open_file *of =kernfs_of (file);
@@ -1035,8 +1075,8 @@ const struct file_operations kernfs_file_fops = { .release = kernfs_fop_release, .poll = kernfs_fop_poll, .fsync = noop_fsync, - .splice_read = copy_splice_read, - .splice_write = iter_file_splice_write, + .splice_read = kernfs_fop_splice_read, + .splice_write = kernfs_fop_splice_write, };/**
fs/sysfs/file.c +18 -0
@@ -252,6 +252,17 @@ static const struct kernfs_ops sysfs_prealloc_kfops_rw = { .prealloc = true, }; +static const struct kernfs_ops sysfs_no_splice_kfops_wo = { + .write = sysfs_kf_write, + .no_splice = true, +}; + +static const struct kernfs_ops sysfs_no_splice_kfops_rw = { + .seq_show = sysfs_kf_seq_show, + .write = sysfs_kf_write, + .no_splice = true, +}; +static const struct kernfs_ops sysfs_bin_kfops_ro = { .read = sysfs_kf_bin_read, };
@@ -296,6 +307,13 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent, ops = &sysfs_prealloc_kfops_ro; else if (sysfs_ops->store) ops = &sysfs_prealloc_kfops_wo; + }else if (mode &SYSFS_NO_SPLICE ) { +if (sysfs_ops->show && sysfs_ops->store) + ops = &sysfs_no_splice_kfops_rw; +else if (sysfs_ops->show) + ops = &sysfs_file_kfops_ro; +else if (sysfs_ops->store) + ops = &sysfs_no_splice_kfops_wo; }else {if (sysfs_ops->show && sysfs_ops->store) ops = &sysfs_file_kfops_rw;
fs/sysfs/group.c +2 -2
@@ -74,11 +74,11 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,continue ; } -WARN (mode & ~(SYSFS_PREALLOC |0664 ), +WARN (mode & ~(SYSFS_PREALLOC |SYSFS_NO_SPLICE |0664 ),"Attribute %s: Invalid permissions 0%o\n" , (*attr)->name, mode); - mode &=SYSFS_PREALLOC |0664 ; + mode &=SYSFS_PREALLOC |SYSFS_NO_SPLICE |0664 ; error =sysfs_add_file_mode_ns (parent, *attr, mode, uid, gid,NULL );if (unlikely (error))
include/linux/kernfs.h +9 -0
@@ -272,6 +272,7 @@ struct kernfs_open_file {char *prealloc_buf;size_t atomic_write_len; +bool no_splice; bool mmapped:1; bool released:1;const struct vm_operations_struct *vm_ops;
@@ -320,6 +321,14 @@ struct kernfs_ops {* ->prealloc. Provide ->read and ->write with ->prealloc. */ bool prealloc; +/* +* "no_splice" makes writable opens refuse splice and gives their +* @of->mutex and seq_file lock separate lockdep classes. Set it +* for a ->write() that may wait for userspace, e.g. through a path +* lookup that triggers an automount, so that the write never runs +* under a pipe mutex. Not supported with ->mmap or ->prealloc. +*/ +bool no_splice;ssize_t (*write)(struct kernfs_open_file *of,char *buf,size_t bytes,loff_t off);
include/linux/sysfs.h +16 -6
@@ -75,12 +75,13 @@ do { \* @is_visible: Optional: Function to return permissions associated with an * attribute of the group. Will be called repeatedly for * each non-binary attribute in the group. Only read/write -* permissions as well as SYSFS_PREALLOC are accepted. Must -* return 0 if an attribute is not visible. The returned -* value will replace static permissions defined in struct -* attribute. Use SYSFS_GROUP_VISIBLE() when assigning this -* callback to specify separate _group_visible() and -* _attr_visible() handlers. +* permissions as well as SYSFS_PREALLOC and SYSFS_NO_SPLICE +* are accepted. Must return 0 if an attribute is not +* visible. The returned value will replace static +* permissions defined in struct attribute. Use +* SYSFS_GROUP_VISIBLE() when assigning this callback to +* specify separate _group_visible() and _attr_visible() +* handlers. * @is_bin_visible: * Optional: Function to return permissions associated with a * binary attribute of the group. Will be called repeatedly
@@ -124,6 +125,8 @@ struct attribute_group {#define SYSFS_PREALLOC 010000#define SYSFS_GROUP_INVISIBLE 020000 +/* Writable opens refuse splice, not for prealloc or binary attributes */ +#define SYSFS_NO_SPLICE 040000/* * DEFINE_SYSFS_GROUP_VISIBLE(name):
@@ -251,6 +254,13 @@ struct attribute_group { .store = _store, \ } +#define __ATTR_NO_SPLICE (_name, _mode, _show, _store) { \ + .attr = {.name = __stringify(_name), \ + .mode = SYSFS_NO_SPLICE | VERIFY_OCTAL_PERMISSIONS(_mode) },\ + .show = _show, \ + .store = _store, \ +} +#define __ATTR_RO_MODE (_name, _mode) { \ .attr = { .name = __stringify(_name), \ .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
Signature · 1 line
2.53.0
Tejun Heo
maintainerRe: [PATCH v3 1/2] kernfs, sysfs: add SYSFS_NO_SPLICE to refuse splice on writable opens
Hello, Krystian.
Quoted 2 lines · Krystian Kaniewski
Read-only opens keep splice support because they cannot reach ->write().
The patch looks fine to me, but I'm not sure whether the read-only exemption is necessary. Is it addressing an actual problem? busybox, for example, just falls back to regular reads when sendfile() fails with -EINVAL, and refusing splice reads on read-write opens while allowing them on read-only ones seems more confusing than anything else. Would it be simpler to refuse splice for all opens of the file?
Thanks.
--
tejun
Re: [PATCH v3 1/2] kernfs, sysfs: add SYSFS_NO_SPLICE to refuse splice on writable opens
On 11/12/26 16:11, Tejun Heo wrote:
Hello Tejun,
Quoted 13 lines
Hello, Krystian.
Quoted 2 lines · Krystian Kaniewski
Read-only opens keep splice support because they cannot reach ->write().
The patch looks fine to me, but I'm not sure whether the read-only exemption is necessary. Is it addressing an actual problem? busybox, for example, just falls back to regular reads when
sendfile()fails with-EINVAL, and refusing splice reads on read-write opens while allowing them on read-only ones seems more confusing than anything else. Would it be simpler to refuse splice for all opens of the file?
Quoted 4 lines
Thanks.
--
tejun
The read-only exemption was only intended to minimize the behavior change. It is not addressing a known user or regression.
I agree that refusing splice for all opens would be simpler and give SYSFS_NO_SPLICE consistent semantics. I can make that change in v4.
Thanks,
Signature · 1 line
Krystian Kaniewski
PATCH v3 2/2 PM: hibernate: refuse splice on /sys/power/resume
In reply to Krystian Kaniewskiresume_store() passes the written string to lookup_bdev() before it tries to parse it as a device number, so the string is resolved as a path relative to the current directory of the writer. The syzbot reproducer writes a relative name from the root of an indirect autofs mount. Autofs writes a mount request to its daemon pipe while kernfs holds of->mutex. Together with a splice into a writable kernfs file, which takes of->mutex inside a pipe mutex, lockdep reports
Chain exists of:
&pipe->mutex --> &of->mutex --> &sbi->pipe_mutexA splice from the daemon pipe into /sys/power/resume deadlocks when the written path triggers a mount request. Mark the attribute with SYSFS_NO_SPLICE so that its writable opens refuse splice and use lockdep classes of their own. Plain read() and write() keep their behavior, and read-only opens retain splice support.
---
v3:
- Use __ATTR_NO_SPLICE after the rename in patch 1. No other change, so Rafael's Acked-by is kept.
v2:
- New patch. Together with patch 1, this replaces the v1 autofs change.
kernel/power/hibernate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/power/hibernate.c +2 -1
@@ -1316,7 +1316,8 @@ static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr,return n; } -power_attr (resume); +static struct kobj_attribute resume_attr = +__ATTR_NO_SPLICE (resume,0644 , resume_show, resume_store);static ssize_t resume_offset_show (struct kobject *kobj,struct kobj_attribute *attr,char *buf)
Signature · 1 line
2.53.0