| Message ID | 171175867930.1987804.1200988399612926993.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/13] fs: add FS_XFLAG_VERITY for verity files | expand |
On Fri, Mar 29, 2024 at 05:33:43PM -0700, Darrick J. Wong wrote: > diff --git a/fs/super.c b/fs/super.c > index 71d9779c42b10..aaa75131f6795 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -637,6 +637,13 @@ void generic_shutdown_super(struct super_block *sb) > sb->s_dio_done_wq = NULL; > } > > +#ifdef CONFIG_FS_VERITY > + if (sb->s_verify_wq) { > + destroy_workqueue(sb->s_verify_wq); > + sb->s_verify_wq = NULL; > + } > +#endif Should it maybe be s_verity_wq with a t? I guess it could be argued that the actual data verification is just part of fsverity, and there could be other workqueues related to fsverity, e.g. one for enabling fsverity. But in practice this is the only one. Also, the runtime name of the workqueue is "fsverity/$s_id", which is more consistent with s_verity_wq than s_verify_wq. > +int __fsverity_init_verify_wq(struct super_block *sb) > +{ > + struct workqueue_struct *wq, *old; > + > + wq = alloc_workqueue("fsverity/%s", WQ_MEM_RECLAIM | WQ_FREEZABLE, 0, > + sb->s_id); > + if (!wq) > + return -ENOMEM; > + > + /* > + * This has to be atomic as readaheads can race to create the > + * workqueue. If someone created workqueue before us, we drop ours. > + */ > + old = cmpxchg(&sb->s_verify_wq, NULL, wq); > + if (old) > + destroy_workqueue(wq); > + return 0; > +} The cmpxchg thing makes no sense because this is only called at mount time, when there is no chance of a race condition. - Eric
On Thu, Apr 04, 2024 at 10:39:53PM -0400, Eric Biggers wrote: > On Fri, Mar 29, 2024 at 05:33:43PM -0700, Darrick J. Wong wrote: > > diff --git a/fs/super.c b/fs/super.c > > index 71d9779c42b10..aaa75131f6795 100644 > > --- a/fs/super.c > > +++ b/fs/super.c > > @@ -637,6 +637,13 @@ void generic_shutdown_super(struct super_block *sb) > > sb->s_dio_done_wq = NULL; > > } > > > > +#ifdef CONFIG_FS_VERITY > > + if (sb->s_verify_wq) { > > + destroy_workqueue(sb->s_verify_wq); > > + sb->s_verify_wq = NULL; > > + } > > +#endif > > Should it maybe be s_verity_wq with a t? > > I guess it could be argued that the actual data verification is just part of > fsverity, and there could be other workqueues related to fsverity, e.g. one for > enabling fsverity. But in practice this is the only one. Yeah. If someone wants to add another wq for enabling verity then we can deal with that > Also, the runtime name of the workqueue is "fsverity/$s_id", which is more > consistent with s_verity_wq than s_verify_wq. Ok, s_verity_wq (with a t) it is. I'll change __fsverity_init_verify_wq to __fsverity_init_wq since we're getting rid of the "verify" wording. > > +int __fsverity_init_verify_wq(struct super_block *sb) > > +{ > > + struct workqueue_struct *wq, *old; > > + > > + wq = alloc_workqueue("fsverity/%s", WQ_MEM_RECLAIM | WQ_FREEZABLE, 0, > > + sb->s_id); > > + if (!wq) > > + return -ENOMEM; > > + > > + /* > > + * This has to be atomic as readaheads can race to create the > > + * workqueue. If someone created workqueue before us, we drop ours. > > + */ > > + old = cmpxchg(&sb->s_verify_wq, NULL, wq); > > + if (old) > > + destroy_workqueue(wq); > > + return 0; > > +} > > The cmpxchg thing makes no sense because this is only called at mount time, when > there is no chance of a race condition. Aha, right. That was a dumb leftover from an earlier version that could turn it on at runtime. Removed. --D > - Eric >
diff --git a/fs/super.c b/fs/super.c index 71d9779c42b10..aaa75131f6795 100644 --- a/fs/super.c +++ b/fs/super.c @@ -637,6 +637,13 @@ void generic_shutdown_super(struct super_block *sb) sb->s_dio_done_wq = NULL; } +#ifdef CONFIG_FS_VERITY + if (sb->s_verify_wq) { + destroy_workqueue(sb->s_verify_wq); + sb->s_verify_wq = NULL; + } +#endif + if (sop->put_super) sop->put_super(sb); diff --git a/fs/verity/verify.c b/fs/verity/verify.c index 0b5e11073e883..0417862d5bd4a 100644 --- a/fs/verity/verify.c +++ b/fs/verity/verify.c @@ -342,6 +342,26 @@ void fsverity_verify_bio(struct bio *bio) EXPORT_SYMBOL_GPL(fsverity_verify_bio); #endif /* CONFIG_BLOCK */ +int __fsverity_init_verify_wq(struct super_block *sb) +{ + struct workqueue_struct *wq, *old; + + wq = alloc_workqueue("fsverity/%s", WQ_MEM_RECLAIM | WQ_FREEZABLE, 0, + sb->s_id); + if (!wq) + return -ENOMEM; + + /* + * This has to be atomic as readaheads can race to create the + * workqueue. If someone created workqueue before us, we drop ours. + */ + old = cmpxchg(&sb->s_verify_wq, NULL, wq); + if (old) + destroy_workqueue(wq); + return 0; +} +EXPORT_SYMBOL_GPL(__fsverity_init_verify_wq); + /** * fsverity_enqueue_verify_work() - enqueue work on the fs-verity workqueue * @work: the work to enqueue diff --git a/include/linux/fs.h b/include/linux/fs.h index 5d1c33573767f..2fc3c2d218ff8 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1230,6 +1230,8 @@ struct super_block { #endif #ifdef CONFIG_FS_VERITY const struct fsverity_operations *s_vop; + /* Completion queue for post read verification */ + struct workqueue_struct *s_verify_wq; #endif #if IS_ENABLED(CONFIG_UNICODE) struct unicode_map *s_encoding; diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h index c3478efd67d62..495708fb1f26a 100644 --- a/include/linux/fsverity.h +++ b/include/linux/fsverity.h @@ -252,6 +252,14 @@ bool fsverity_verify_blocks(struct folio *folio, size_t len, size_t offset); void fsverity_verify_bio(struct bio *bio); void fsverity_enqueue_verify_work(struct work_struct *work); +int __fsverity_init_verify_wq(struct super_block *sb); +static inline int fsverity_init_verify_wq(struct super_block *sb) +{ + if (sb->s_verify_wq) + return 0; + return __fsverity_init_verify_wq(sb); +} + #else /* !CONFIG_FS_VERITY */ static inline struct fsverity_info *fsverity_get_info(const struct inode *inode) @@ -329,6 +337,11 @@ static inline void fsverity_enqueue_verify_work(struct work_struct *work) WARN_ON_ONCE(1); } +static inline int fsverity_init_verify_wq(struct super_block *sb) +{ + return -EOPNOTSUPP; +} + #endif /* !CONFIG_FS_VERITY */ static inline bool fsverity_verify_folio(struct folio *folio)