| Message ID | 20200728234513.1956039-1-ytht.net@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Add filesystem attribute in sysfs control dir. | expand |
On Tue, Jul 28, 2020 at 4:45 PM Lepton Wu <ytht.net@gmail.com> wrote: > > With this, user space can have more control to just abort some kind of > fuse connections. Currently, in Android, it will write to abort file > to abort all fuse connections while in some cases, we'd like to keep > some fuse connections. This can help that. > > Signed-off-by: Lepton Wu <ytht.net@gmail.com> > --- > fs/fuse/control.c | 31 ++++++++++++++++++++++++++++++- > fs/fuse/fuse_i.h | 2 +- > 2 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/fs/fuse/control.c b/fs/fuse/control.c > index c23f6f243ad42..85a56d2de50d5 100644 > --- a/fs/fuse/control.c > +++ b/fs/fuse/control.c > @@ -64,6 +64,27 @@ static ssize_t fuse_conn_waiting_read(struct file *file, char __user *buf, > return simple_read_from_buffer(buf, len, ppos, tmp, size); > } > > +static ssize_t fuse_conn_file_system_read(struct file *file, char __user *buf, > + size_t len, loff_t *ppos) > +{ > + char tmp[32]; > + size_t size; > + > + if (!*ppos) { > + struct fuse_conn *fc = fuse_ctl_file_conn_get(file); > + > + if (!fc) > + return 0; > + if (fc->sb && fc->sb->s_type) > + file->private_data = (void *)fc->sb->s_type->name; > + else > + file->private_data = "(NULL)"; > + fuse_conn_put(fc); > + } > + size = sprintf(tmp, "%.30s\n", (char *)file->private_data); > + return simple_read_from_buffer(buf, len, ppos, tmp, size); > +} > + > static ssize_t fuse_conn_limit_read(struct file *file, char __user *buf, > size_t len, loff_t *ppos, unsigned val) > { > @@ -217,6 +238,12 @@ static const struct file_operations fuse_conn_congestion_threshold_ops = { > .llseek = no_llseek, > }; > > +static const struct file_operations fuse_conn_file_system_ops = { > + .open = nonseekable_open, > + .read = fuse_conn_file_system_read, > + .llseek = no_llseek, > +}; > + > static struct dentry *fuse_ctl_add_dentry(struct dentry *parent, > struct fuse_conn *fc, > const char *name, > @@ -285,7 +312,9 @@ int fuse_ctl_add_conn(struct fuse_conn *fc) > 1, NULL, &fuse_conn_max_background_ops) || > !fuse_ctl_add_dentry(parent, fc, "congestion_threshold", > S_IFREG | 0600, 1, NULL, > - &fuse_conn_congestion_threshold_ops)) > + &fuse_conn_congestion_threshold_ops) || > + !fuse_ctl_add_dentry(parent, fc, "filesystem", S_IFREG | 0400, 1, > + NULL, &fuse_conn_file_system_ops)) > goto err; > > return 0; > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index 740a8a7d7ae6f..59390ed37bbad 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -45,7 +45,7 @@ > #define FUSE_NAME_MAX 1024 > > /** Number of dentries for each connection in the control filesystem */ > -#define FUSE_CTL_NUM_DENTRIES 5 > +#define FUSE_CTL_NUM_DENTRIES 6 > > /** List of active connections */ > extern struct list_head fuse_conn_list; > -- > 2.28.0.163.g6104cc2f0b6-goog > Ping this old patch. I just checked and it still applies cleanly to HEAD. Can we have this merged? Thanks!
On Wed, 29 Jul 2020 at 01:45, Lepton Wu <ytht.net@gmail.com> wrote: > > With this, user space can have more control to just abort some kind of > fuse connections. Currently, in Android, it will write to abort file > to abort all fuse connections while in some cases, we'd like to keep > some fuse connections. This can help that. You can grep the same info from /proc/self/mountinfo. Why does that not work? Thanks, Miklos
On Mon, Aug 22, 2022 at 6:37 AM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Wed, 29 Jul 2020 at 01:45, Lepton Wu <ytht.net@gmail.com> wrote: > > > > With this, user space can have more control to just abort some kind of > > fuse connections. Currently, in Android, it will write to abort file > > to abort all fuse connections while in some cases, we'd like to keep > > some fuse connections. This can help that. > > You can grep the same info from /proc/self/mountinfo. Why does that not work? Hi Miklos, thanks for this hint. That will work. But the code in user space will be more complicated and not straightforward. For now, I can see 2 issues with mountinfo: 1. one connection could have multiple entries under /proc/self/mountinfo if there are bind mounts, user space code needs to handle that. 2. /proc/self/mountinfo is limited by namespace, so in theory, some connection under /sys/fs/fuse/connections could be missed in it. While this isn't an issue for the current android code, maybe it could get broken in the future. Overall, I am feeling my kernel patch is a straightforward and simple solution and it's just one more attribute under /sys/fs/fuse/connections, may I know if there are any downsides to doing this? Thanks! > > Thanks, > Miklos
On Wed, 24 Aug 2022 at 21:00, lepton <ytht.net@gmail.com> wrote: > > On Mon, Aug 22, 2022 at 6:37 AM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > On Wed, 29 Jul 2020 at 01:45, Lepton Wu <ytht.net@gmail.com> wrote: > > > > > > With this, user space can have more control to just abort some kind of > > > fuse connections. Currently, in Android, it will write to abort file > > > to abort all fuse connections while in some cases, we'd like to keep > > > some fuse connections. This can help that. > > > > You can grep the same info from /proc/self/mountinfo. Why does that not work? > Hi Miklos, thanks for this hint. That will work. But the code in user > space will be more complicated and not straightforward. > For now, I can see 2 issues with mountinfo: > 1. one connection could have multiple entries under > /proc/self/mountinfo if there are bind mounts, user space code needs > to handle that. Using the first match should be okay, bind mounts will have the same filesystem type. > 2. /proc/self/mountinfo is limited by namespace, so in theory, some > connection under /sys/fs/fuse/connections could be missed in it. True. OTOH this could be a security issue as well, since the point of namespaces is to hide information. > While this isn't an issue > for the current android code, maybe it could get broken in the future. > > Overall, I am feeling my kernel patch is a straightforward and simple > solution and it's just one more attribute under > /sys/fs/fuse/connections, may I know if there are any downsides to > doing this? Your patch adds an ad-hoc interface for a very specific purpose with likely no other users. As long as this information can be gotten in other ways there's no compelling reason to add a new interface. Thanks, Miklos
diff --git a/fs/fuse/control.c b/fs/fuse/control.c index c23f6f243ad42..85a56d2de50d5 100644 --- a/fs/fuse/control.c +++ b/fs/fuse/control.c @@ -64,6 +64,27 @@ static ssize_t fuse_conn_waiting_read(struct file *file, char __user *buf, return simple_read_from_buffer(buf, len, ppos, tmp, size); } +static ssize_t fuse_conn_file_system_read(struct file *file, char __user *buf, + size_t len, loff_t *ppos) +{ + char tmp[32]; + size_t size; + + if (!*ppos) { + struct fuse_conn *fc = fuse_ctl_file_conn_get(file); + + if (!fc) + return 0; + if (fc->sb && fc->sb->s_type) + file->private_data = (void *)fc->sb->s_type->name; + else + file->private_data = "(NULL)"; + fuse_conn_put(fc); + } + size = sprintf(tmp, "%.30s\n", (char *)file->private_data); + return simple_read_from_buffer(buf, len, ppos, tmp, size); +} + static ssize_t fuse_conn_limit_read(struct file *file, char __user *buf, size_t len, loff_t *ppos, unsigned val) { @@ -217,6 +238,12 @@ static const struct file_operations fuse_conn_congestion_threshold_ops = { .llseek = no_llseek, }; +static const struct file_operations fuse_conn_file_system_ops = { + .open = nonseekable_open, + .read = fuse_conn_file_system_read, + .llseek = no_llseek, +}; + static struct dentry *fuse_ctl_add_dentry(struct dentry *parent, struct fuse_conn *fc, const char *name, @@ -285,7 +312,9 @@ int fuse_ctl_add_conn(struct fuse_conn *fc) 1, NULL, &fuse_conn_max_background_ops) || !fuse_ctl_add_dentry(parent, fc, "congestion_threshold", S_IFREG | 0600, 1, NULL, - &fuse_conn_congestion_threshold_ops)) + &fuse_conn_congestion_threshold_ops) || + !fuse_ctl_add_dentry(parent, fc, "filesystem", S_IFREG | 0400, 1, + NULL, &fuse_conn_file_system_ops)) goto err; return 0; diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 740a8a7d7ae6f..59390ed37bbad 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -45,7 +45,7 @@ #define FUSE_NAME_MAX 1024 /** Number of dentries for each connection in the control filesystem */ -#define FUSE_CTL_NUM_DENTRIES 5 +#define FUSE_CTL_NUM_DENTRIES 6 /** List of active connections */ extern struct list_head fuse_conn_list;
With this, user space can have more control to just abort some kind of fuse connections. Currently, in Android, it will write to abort file to abort all fuse connections while in some cases, we'd like to keep some fuse connections. This can help that. Signed-off-by: Lepton Wu <ytht.net@gmail.com> --- fs/fuse/control.c | 31 ++++++++++++++++++++++++++++++- fs/fuse/fuse_i.h | 2 +- 2 files changed, 31 insertions(+), 2 deletions(-)