vfs: change iterate_supers callback to take an int arg instead of a void *
Subsystems (4)
PATCH 1/1 vfs: change iterate_supers callback to take an int arg instead of a void *
From: Jeff Layton <[email protected]>
All of the callback functions for iterate_supers either ignore the opaque argument, or dereference the pointer only to fetch the int to which it points.
Change iterate_supers to pass an opaque int arg to the callback instead of a void pointer.
--- fs/drop_caches.c | 4 ++-- fs/quota/quota.c | 6 ++---- fs/super.c | 3 ++- fs/sync.c | 32 ++++++++++++++------------------ include/linux/fs.h | 2 +- mm/cleancache.c | 4 ++-- security/selinux/hooks.c | 4 ++-- 7 files changed, 25 insertions(+), 30 deletions(-)
fs/drop_caches.c +2 -2
@@ -14,7 +14,7 @@/* A global variable is a bit ugly, but it keeps the code simple */ int sysctl_drop_caches; -static void drop_pagecache_sb (struct super_block *sb,void *unused) +static void drop_pagecache_sb (struct super_block *sb,int unused) {struct inode *inode, *toput_inode =NULL ;
@@ -52,7 +52,7 @@ int drop_caches_sysctl_handler(struct ctl_table *table, int write,static int stfu;if (sysctl_drop_caches &1 ) { -iterate_supers (drop_pagecache_sb,NULL ); +iterate_supers (drop_pagecache_sb,0 );count_vm_event (DROP_PAGECACHE ); }if (sysctl_drop_caches &2 ) {
fs/quota/quota.c +2 -4
@@ -46,10 +46,8 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd,return security_quotactl (cmd, type, id, sb); } -static void quota_sync_one (struct super_block *sb,void *arg) +static void quota_sync_one (struct super_block *sb,int type) { -int type = *(int *)arg; -if (sb->s_qcop && sb->s_qcop->quota_sync && (sb->s_quota_types & (1 << type))) sb->s_qcop->quota_sync (sb, type);
@@ -63,7 +61,7 @@ static int quota_sync_all(int type)return -EINVAL ; ret =security_quotactl (Q_SYNC , type,0 ,NULL );if (!ret) -iterate_supers (quota_sync_one, &type); +iterate_supers (quota_sync_one, type);return ret; }
fs/super.c +2 -1
@@ -596,6 +596,7 @@ static void __iterate_supers(void (*f)(struct super_block *))__put_super (p);spin_unlock (&sb_lock); } +/** * iterate_supers - call function for all active superblocks * @f: function to call
@@ -604,7 +605,7 @@ static void __iterate_supers(void (*f)(struct super_block *))* Scans the superblock list and calls given function, passing it * locked superblock and given argument. */ -void iterate_supers (void (*f)(struct super_block *,void *),void *arg) +void iterate_supers (void (*f)(struct super_block *,int ),int arg) {struct super_block *sb, *p =NULL ;
fs/sync.c +14 -18
@@ -68,16 +68,16 @@ int sync_filesystem(struct super_block *sb) }EXPORT_SYMBOL (sync_filesystem); -static void sync_inodes_one_sb (struct super_block *sb,void *arg) +static void sync_inodes_one_sb (struct super_block *sb,int unused) {if (!sb_rdonly (sb))sync_inodes_sb (sb); } -static void sync_fs_one_sb (struct super_block *sb,void *arg) +static void sync_fs_one_sb (struct super_block *sb,int arg) {if (!sb_rdonly (sb) && sb->s_op->sync_fs) - sb->s_op->sync_fs (sb, *(int *)arg); + sb->s_op->sync_fs (sb, arg); }static void fdatawrite_one_bdev (struct block_device *bdev,void *arg)
@@ -107,14 +107,12 @@ static void fdatawait_one_bdev(struct block_device *bdev, void *arg)*/ void ksys_sync (void ) { -int nowait =0 , wait =1 ; -wakeup_flusher_threads (WB_REASON_SYNC ); -iterate_supers (sync_inodes_one_sb,NULL ); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &wait); -iterate_bdevs (fdatawrite_one_bdev,NULL ); -iterate_bdevs (fdatawait_one_bdev,NULL ); +iterate_supers (sync_inodes_one_sb,0 ); +iterate_supers (sync_fs_one_sb,0 ); +iterate_supers (sync_fs_one_sb,1 ); +iterate_bdevs (fdatawrite_one_bdev,0 ); +iterate_bdevs (fdatawait_one_bdev,0 );if (unlikely (laptop_mode))laptop_sync_completion (); }
@@ -127,18 +125,16 @@ SYSCALL_DEFINE0(sync)static void do_sync_work (struct work_struct *work) { -int nowait =0 ; -/* * Sync twice to reduce the possibility we skipped some inodes / pages * because they were temporarily locked */ -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_bdevs (fdatawrite_one_bdev,NULL ); -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_bdevs (fdatawrite_one_bdev,NULL ); +iterate_supers (sync_inodes_one_sb,0 ); +iterate_supers (sync_fs_one_sb,0 ); +iterate_bdevs (fdatawrite_one_bdev,0 ); +iterate_supers (sync_inodes_one_sb,0 ); +iterate_supers (sync_fs_one_sb,0 ); +iterate_bdevs (fdatawrite_one_bdev,0 );printk ("Emergency Sync complete\n" );kfree (work); }
include/linux/fs.h +1 -1
@@ -3115,7 +3115,7 @@ extern struct super_block *get_super_exclusive_thawed(struct block_device *bdev)extern struct super_block *get_active_super (struct block_device *bdev);extern void drop_super (struct super_block *sb);extern void drop_super_exclusive (struct super_block *sb); -extern void iterate_supers (void (*)(struct super_block *,void *),void *); +extern void iterate_supers (void (*)(struct super_block *,int ),int );extern void iterate_supers_type (struct file_system_type *,void (*)(struct super_block *,void *),void *);
mm/cleancache.c +2 -2
@@ -34,7 +34,7 @@ static u64 cleancache_failed_gets;static u64 cleancache_puts;static u64 cleancache_invalidates; -static void cleancache_register_ops_sb (struct super_block *sb,void *unused) +static void cleancache_register_ops_sb (struct super_block *sb,int unused) {switch (sb->cleancache_poolid) {case CLEANCACHE_NO_BACKEND :
@@ -105,7 +105,7 @@ int cleancache_register_ops(const struct cleancache_ops *ops)* until the corresponding ->init_fs has been actually called and * cleancache_ops has been set. */ -iterate_supers (cleancache_register_ops_sb,NULL ); +iterate_supers (cleancache_register_ops_sb,0 );return 0 ; }EXPORT_SYMBOL (cleancache_register_ops);
security/selinux/hooks.c +2 -2
@@ -7138,7 +7138,7 @@ static __init int selinux_init(void)return 0 ; } -static void delayed_superblock_init (struct super_block *sb,void *unused) +static void delayed_superblock_init (struct super_block *sb,int unused) {superblock_doinit (sb,NULL ); }
@@ -7149,7 +7149,7 @@ void selinux_complete_init(void)/* Set up any superblocks initialized prior to the policy load. */ printk (KERN_DEBUG "SELinux: Setting up existing superblocks.\n" ); -iterate_supers (delayed_superblock_init,NULL ); +iterate_supers (delayed_superblock_init,0 ); }/* SELinux requires early initialization in order to label
Signature · 1 line
2.17.0
Jan Kara
reviewerQuoted 10 lines · Jeff Layton
From: Jeff Layton <[email protected]>
All of the callback functions for
iterate_superseither ignore the opaque argument, or dereference the pointer only to fetch the int to which it points.Change
iterate_supersto pass an opaque int arg to the callback instead of a void pointer.Signed-off-by Jeff Layton <[email protected]>
Hum, I'm not sure if Luis didn't have any plan with the argument in his fs-freeze-during-hibernate series. Luis?
Honza
Quoted 208 lines
--- fs/drop_caches.c | 4 ++-- fs/quota/quota.c | 6 ++---- fs/super.c | 3 ++- fs/sync.c | 32 ++++++++++++++------------------ include/linux/fs.h | 2 +- mm/cleancache.c | 4 ++-- security/selinux/hooks.c | 4 ++-- 7 files changed, 25 insertions(+), 30 deletions(-)fs/drop_caches.c +2
-2@@ -14,7 +14,7 @@/* A global variable is a bit ugly, but it keeps the code simple */ int sysctl_drop_caches; -static void drop_pagecache_sb (struct super_block *sb,void *unused) +static void drop_pagecache_sb (struct super_block *sb,int unused) {struct inode *inode, *toput_inode =NULL ;@@ -52,7 +52,7 @@ int drop_caches_sysctl_handler(struct ctl_table *table, int write,static int stfu;if (sysctl_drop_caches &1 ) { -iterate_supers (drop_pagecache_sb,NULL ); +iterate_supers (drop_pagecache_sb,0 );count_vm_event (DROP_PAGECACHE ); }if (sysctl_drop_caches &2 ) {fs/quota/quota.c +2
-4@@ -46,10 +46,8 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd,return security_quotactl (cmd, type, id, sb); } -static void quota_sync_one (struct super_block *sb,void *arg) +static void quota_sync_one (struct super_block *sb,int type) { -int type = *(int *)arg; -if (sb->s_qcop && sb->s_qcop->quota_sync && (sb->s_quota_types & (1 << type))) sb->s_qcop->quota_sync (sb, type);@@ -63,7 +61,7 @@ static int quota_sync_all(int type)return -EINVAL ; ret =security_quotactl (Q_SYNC , type,0 ,NULL );if (!ret) -iterate_supers (quota_sync_one, &type); +iterate_supers (quota_sync_one, type);return ret; }fs/super.c +2
-1@@ -596,6 +596,7 @@ static void __iterate_supers(void (*f)(struct super_block *))__put_super (p);spin_unlock (&sb_lock); } +/** * iterate_supers - call function for all active superblocks * @f: function to call @@ -604,7 +605,7 @@ static void __iterate_supers(void (*f)(struct super_block *))* Scans the superblock list and calls given function, passing it * locked superblock and given argument. */ -void iterate_supers (void (*f)(struct super_block *,void *),void *arg) +void iterate_supers (void (*f)(struct super_block *,int ),int arg) {struct super_block *sb, *p =NULL ;fs/sync.c +14
-18@@ -68,16 +68,16 @@ int sync_filesystem(struct super_block *sb) }EXPORT_SYMBOL (sync_filesystem); -static void sync_inodes_one_sb (struct super_block *sb,void *arg) +static void sync_inodes_one_sb (struct super_block *sb,int unused) {if (!sb_rdonly (sb))sync_inodes_sb (sb); } -static void sync_fs_one_sb (struct super_block *sb,void *arg) +static void sync_fs_one_sb (struct super_block *sb,int arg) {if (!sb_rdonly (sb) && sb->s_op->sync_fs) - sb->s_op->sync_fs (sb, *(int *)arg); + sb->s_op->sync_fs (sb, arg); }static void fdatawrite_one_bdev (struct block_device *bdev,void *arg)@@ -107,14 +107,12 @@ static void fdatawait_one_bdev(struct block_device *bdev, void *arg)*/ void ksys_sync (void ) { -int nowait =0 , wait =1 ; -wakeup_flusher_threads (WB_REASON_SYNC ); -iterate_supers (sync_inodes_one_sb,NULL ); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &wait); -iterate_bdevs (fdatawrite_one_bdev,NULL ); -iterate_bdevs (fdatawait_one_bdev,NULL ); +iterate_supers (sync_inodes_one_sb,0 ); +iterate_supers (sync_fs_one_sb,0 ); +iterate_supers (sync_fs_one_sb,1 ); +iterate_bdevs (fdatawrite_one_bdev,0 ); +iterate_bdevs (fdatawait_one_bdev,0 );if (unlikely (laptop_mode))laptop_sync_completion (); }@@ -127,18 +125,16 @@ SYSCALL_DEFINE0(sync)static void do_sync_work (struct work_struct *work) { -int nowait =0 ; -/* * Sync twice to reduce the possibility we skipped some inodes / pages * because they were temporarily locked */ -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_bdevs (fdatawrite_one_bdev,NULL ); -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_bdevs (fdatawrite_one_bdev,NULL ); +iterate_supers (sync_inodes_one_sb,0 ); +iterate_supers (sync_fs_one_sb,0 ); +iterate_bdevs (fdatawrite_one_bdev,0 ); +iterate_supers (sync_inodes_one_sb,0 ); +iterate_supers (sync_fs_one_sb,0 ); +iterate_bdevs (fdatawrite_one_bdev,0 );printk ("Emergency Sync complete\n" );kfree (work); }include/linux/fs.h +1
-1@@ -3115,7 +3115,7 @@ extern struct super_block *get_super_exclusive_thawed(struct block_device *bdev)extern struct super_block *get_active_super (struct block_device *bdev);extern void drop_super (struct super_block *sb);extern void drop_super_exclusive (struct super_block *sb); -extern void iterate_supers (void (*)(struct super_block *,void *),void *); +extern void iterate_supers (void (*)(struct super_block *,int ),int );extern void iterate_supers_type (struct file_system_type *,void (*)(struct super_block *,void *),void *);mm/cleancache.c +2
-2@@ -34,7 +34,7 @@ static u64 cleancache_failed_gets;static u64 cleancache_puts;static u64 cleancache_invalidates; -static void cleancache_register_ops_sb (struct super_block *sb,void *unused) +static void cleancache_register_ops_sb (struct super_block *sb,int unused) {switch (sb->cleancache_poolid) {case CLEANCACHE_NO_BACKEND :@@ -105,7 +105,7 @@ int cleancache_register_ops(const struct cleancache_ops *ops)* until the corresponding ->init_fs has been actually called and * cleancache_ops has been set. */ -iterate_supers (cleancache_register_ops_sb,NULL ); +iterate_supers (cleancache_register_ops_sb,0 );return 0 ; }EXPORT_SYMBOL (cleancache_register_ops);security/selinux/hooks.c +2
-2@@ -7138,7 +7138,7 @@ static __init int selinux_init(void)return 0 ; } -static void delayed_superblock_init (struct super_block *sb,void *unused) +static void delayed_superblock_init (struct super_block *sb,int unused) {superblock_doinit (sb,NULL ); }@@ -7149,7 +7149,7 @@ void selinux_complete_init(void)/* Set up any superblocks initialized prior to the policy load. */ printk (KERN_DEBUG "SELinux: Setting up existing superblocks.\n" ); -iterate_supers (delayed_superblock_init,NULL ); +iterate_supers (delayed_superblock_init,0 ); }/* SELinux requires early initialization in order to label Signature · 1 line
2.17.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 10 lines · Jan Kara
Quoted 9 lines · Jeff Layton
From: Jeff Layton <[email protected]>
All of the callback functions for
iterate_superseither ignore the opaque argument, or dereference the pointer only to fetch the int to which it points.Change
iterate_supersto pass an opaque int arg to the callback instead of a void pointer.
Quoted 7 lines
Quoted 1 line
Signed-off-by Jeff Layton <[email protected]>Hum, I'm not sure if Luis didn't have any plan with the argument in his fs-freeze-during-hibernate series. Luis?
Honza
No worries... Al already NAK'ed this over IRC. I'm reworking the patch to avoid dereferencing in the existing callers, but leave the callback prototypes intact. I'll send a v2 patch shortly...
Thanks,
Jeff
Quoted 208 lines
Quoted 208 lines
--- fs/drop_caches.c | 4 ++-- fs/quota/quota.c | 6 ++---- fs/super.c | 3 ++- fs/sync.c | 32 ++++++++++++++------------------ include/linux/fs.h | 2 +- mm/cleancache.c | 4 ++-- security/selinux/hooks.c | 4 ++-- 7 files changed, 25 insertions(+), 30 deletions(-)fs/drop_caches.c +2
-2@@ -14,7 +14,7 @@/* A global variable is a bit ugly, but it keeps the code simple */ int sysctl_drop_caches; -static void drop_pagecache_sb (struct super_block *sb,void *unused) +static void drop_pagecache_sb (struct super_block *sb,int unused) {struct inode *inode, *toput_inode =NULL ;@@ -52,7 +52,7 @@ int drop_caches_sysctl_handler(struct ctl_table *table, int write,static int stfu;if (sysctl_drop_caches &1 ) { -iterate_supers (drop_pagecache_sb,NULL ); +iterate_supers (drop_pagecache_sb,0 );count_vm_event (DROP_PAGECACHE ); }if (sysctl_drop_caches &2 ) {fs/quota/quota.c +2
-4@@ -46,10 +46,8 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd,return security_quotactl (cmd, type, id, sb); } -static void quota_sync_one (struct super_block *sb,void *arg) +static void quota_sync_one (struct super_block *sb,int type) { -int type = *(int *)arg; -if (sb->s_qcop && sb->s_qcop->quota_sync && (sb->s_quota_types & (1 << type))) sb->s_qcop->quota_sync (sb, type);@@ -63,7 +61,7 @@ static int quota_sync_all(int type)return -EINVAL ; ret =security_quotactl (Q_SYNC , type,0 ,NULL );if (!ret) -iterate_supers (quota_sync_one, &type); +iterate_supers (quota_sync_one, type);return ret; }fs/super.c +2
-1@@ -596,6 +596,7 @@ static void __iterate_supers(void (*f)(struct super_block *))__put_super (p);spin_unlock (&sb_lock); } +/** * iterate_supers - call function for all active superblocks * @f: function to call @@ -604,7 +605,7 @@ static void __iterate_supers(void (*f)(struct super_block *))* Scans the superblock list and calls given function, passing it * locked superblock and given argument. */ -void iterate_supers (void (*f)(struct super_block *,void *),void *arg) +void iterate_supers (void (*f)(struct super_block *,int ),int arg) {struct super_block *sb, *p =NULL ;fs/sync.c +14
-18@@ -68,16 +68,16 @@ int sync_filesystem(struct super_block *sb) }EXPORT_SYMBOL (sync_filesystem); -static void sync_inodes_one_sb (struct super_block *sb,void *arg) +static void sync_inodes_one_sb (struct super_block *sb,int unused) {if (!sb_rdonly (sb))sync_inodes_sb (sb); } -static void sync_fs_one_sb (struct super_block *sb,void *arg) +static void sync_fs_one_sb (struct super_block *sb,int arg) {if (!sb_rdonly (sb) && sb->s_op->sync_fs) - sb->s_op->sync_fs (sb, *(int *)arg); + sb->s_op->sync_fs (sb, arg); }static void fdatawrite_one_bdev (struct block_device *bdev,void *arg)@@ -107,14 +107,12 @@ static void fdatawait_one_bdev(struct block_device *bdev, void *arg)*/ void ksys_sync (void ) { -int nowait =0 , wait =1 ; -wakeup_flusher_threads (WB_REASON_SYNC ); -iterate_supers (sync_inodes_one_sb,NULL ); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &wait); -iterate_bdevs (fdatawrite_one_bdev,NULL ); -iterate_bdevs (fdatawait_one_bdev,NULL ); +iterate_supers (sync_inodes_one_sb,0 ); +iterate_supers (sync_fs_one_sb,0 ); +iterate_supers (sync_fs_one_sb,1 ); +iterate_bdevs (fdatawrite_one_bdev,0 ); +iterate_bdevs (fdatawait_one_bdev,0 );if (unlikely (laptop_mode))laptop_sync_completion (); }@@ -127,18 +125,16 @@ SYSCALL_DEFINE0(sync)static void do_sync_work (struct work_struct *work) { -int nowait =0 ; -/* * Sync twice to reduce the possibility we skipped some inodes / pages * because they were temporarily locked */ -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_bdevs (fdatawrite_one_bdev,NULL ); -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_bdevs (fdatawrite_one_bdev,NULL ); +iterate_supers (sync_inodes_one_sb,0 ); +iterate_supers (sync_fs_one_sb,0 ); +iterate_bdevs (fdatawrite_one_bdev,0 ); +iterate_supers (sync_inodes_one_sb,0 ); +iterate_supers (sync_fs_one_sb,0 ); +iterate_bdevs (fdatawrite_one_bdev,0 );printk ("Emergency Sync complete\n" );kfree (work); }include/linux/fs.h +1
-1@@ -3115,7 +3115,7 @@ extern struct super_block *get_super_exclusive_thawed(struct block_device *bdev)extern struct super_block *get_active_super (struct block_device *bdev);extern void drop_super (struct super_block *sb);extern void drop_super_exclusive (struct super_block *sb); -extern void iterate_supers (void (*)(struct super_block *,void *),void *); +extern void iterate_supers (void (*)(struct super_block *,int ),int );extern void iterate_supers_type (struct file_system_type *,void (*)(struct super_block *,void *),void *);mm/cleancache.c +2
-2@@ -34,7 +34,7 @@ static u64 cleancache_failed_gets;static u64 cleancache_puts;static u64 cleancache_invalidates; -static void cleancache_register_ops_sb (struct super_block *sb,void *unused) +static void cleancache_register_ops_sb (struct super_block *sb,int unused) {switch (sb->cleancache_poolid) {case CLEANCACHE_NO_BACKEND :@@ -105,7 +105,7 @@ int cleancache_register_ops(const struct cleancache_ops *ops)* until the corresponding ->init_fs has been actually called and * cleancache_ops has been set. */ -iterate_supers (cleancache_register_ops_sb,NULL ); +iterate_supers (cleancache_register_ops_sb,0 );return 0 ; }EXPORT_SYMBOL (cleancache_register_ops);security/selinux/hooks.c +2
-2@@ -7138,7 +7138,7 @@ static __init int selinux_init(void)return 0 ; } -static void delayed_superblock_init (struct super_block *sb,void *unused) +static void delayed_superblock_init (struct super_block *sb,int unused) {superblock_doinit (sb,NULL ); }@@ -7149,7 +7149,7 @@ void selinux_complete_init(void)/* Set up any superblocks initialized prior to the policy load. */ printk (KERN_DEBUG "SELinux: Setting up existing superblocks.\n" ); -iterate_supers (delayed_superblock_init,NULL ); +iterate_supers (delayed_superblock_init,0 ); }/* SELinux requires early initialization in order to label Signature · 1 line
2.17.0
Signature · 1 line
Jeff Layton <[email protected]>
PATCH v2 1/1 vfs: avoid dereferencing pointers in iterate_supers callbacks
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
All of the callback functions for iterate_supers either ignore the opaque argument, or dereference the pointer only to fetch the int to which it points.
Change quota_sync_one to just cast the int from the pointer, and change sync_fs_one_sb to just use a NULL/non-NULL pointer as a flag.
--- fs/quota/quota.c | 4 ++-- fs/sync.c | 20 +++++++++----------- 2 files changed, 11 insertions(+), 13 deletions(-)
fs/quota/quota.c +2 -2
@@ -48,7 +48,7 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd,static void quota_sync_one (struct super_block *sb,void *arg) { -int type = *(int *)arg; +int type = (unsigned long )arg;if (sb->s_qcop && sb->s_qcop->quota_sync && (sb->s_quota_types & (1 << type)))
@@ -63,7 +63,7 @@ static int quota_sync_all(int type)return -EINVAL ; ret =security_quotactl (Q_SYNC , type,0 ,NULL );if (!ret) -iterate_supers (quota_sync_one, &type); +iterate_supers (quota_sync_one, (void *)((unsigned long )type));return ret; }
fs/sync.c +9 -11
@@ -76,8 +76,10 @@ static void sync_inodes_one_sb(struct super_block *sb, void *arg)static void sync_fs_one_sb (struct super_block *sb,void *arg) { +int wait = arg ?1 :0 ; +if (!sb_rdonly (sb) && sb->s_op->sync_fs) - sb->s_op->sync_fs (sb, *(int *)arg); + sb->s_op->sync_fs (sb, wait); }static void fdatawrite_one_bdev (struct block_device *bdev,void *arg)
@@ -107,12 +109,10 @@ static void fdatawait_one_bdev(struct block_device *bdev, void *arg)*/ void ksys_sync (void ) { -int nowait =0 , wait =1 ; -wakeup_flusher_threads (WB_REASON_SYNC );iterate_supers (sync_inodes_one_sb,NULL ); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &wait); +iterate_supers (sync_fs_one_sb,NULL ); +iterate_supers (sync_fs_one_sb, (void *)1UL );iterate_bdevs (fdatawrite_one_bdev,NULL );iterate_bdevs (fdatawait_one_bdev,NULL );if (unlikely (laptop_mode))
@@ -127,17 +127,15 @@ SYSCALL_DEFINE0(sync)static void do_sync_work (struct work_struct *work) { -int nowait =0 ; -/* * Sync twice to reduce the possibility we skipped some inodes / pages * because they were temporarily locked */ -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); +iterate_supers (sync_inodes_one_sb,NULL ); +iterate_supers (sync_fs_one_sb,NULL );iterate_bdevs (fdatawrite_one_bdev,NULL ); -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); +iterate_supers (sync_inodes_one_sb,NULL ); +iterate_supers (sync_fs_one_sb,NULL );iterate_bdevs (fdatawrite_one_bdev,NULL );printk ("Emergency Sync complete\n" );kfree (work);
Signature · 1 line
2.17.0
Re: [PATCH v2] vfs: avoid dereferencing pointers in iterate_supers callbacks
fs/sync.c static void fdatawait_one_bdev(struct block_device *bdev, void *arg) · line 115
Quoted 12 lines · Jeff Layton
fs/sync.c +2
-4@@ -107,12 +109,10 @@ static void fdatawait_one_bdev(struct block_device *bdev, void *arg)*/ void ksys_sync (void ) { -int nowait =0 , wait =1 ; -wakeup_flusher_threads (WB_REASON_SYNC );iterate_supers (sync_inodes_one_sb,NULL ); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &wait); +iterate_supers (sync_fs_one_sb,NULL ); +iterate_supers (sync_fs_one_sb, (void *)1UL );
I think this is actually less clear. How about:
void *wait = (void *)1UL; iterate_supers(sync_fs_one_sb, !wait); iterate_supers(sync_fs_one_sb, wait);
Re: [PATCH v2] vfs: avoid dereferencing pointers in iterate_supers callbacks
Quoted 16 lines · Matthew Wilcox
fs/sync.c static void fdatawait_one_bdev(struct block_device *bdev, void *arg) · line 115
Quoted 12 lines · Jeff Layton
fs/sync.c +2
-4@@ -107,12 +109,10 @@ static void fdatawait_one_bdev(struct block_device *bdev, void *arg)*/ void ksys_sync (void ) { -int nowait =0 , wait =1 ; -wakeup_flusher_threads (WB_REASON_SYNC );iterate_supers (sync_inodes_one_sb,NULL ); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &wait); +iterate_supers (sync_fs_one_sb,NULL ); +iterate_supers (sync_fs_one_sb, (void *)1UL );I think this is actually less clear. How about:
Quoted 5 lines
void *wait = (void *)1UL; iterate_supers(sync_fs_one_sb, !wait); iterate_supers(sync_fs_one_sb, wait);
Sure, I could do that. We could also keep the wait/nowait vars too, I guess.
PATCH v3 1/1 vfs: avoid dereferencing pointers in iterate_supers callbacks
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
All of the callback functions for iterate_supers either ignore the opaque argument, or dereference the pointer only to fetch the int to which it points.
Change quota_sync_one to just cast the int from the pointer, and change sync_fs_one_sb to just use a NULL/non-NULL pointer as a flag.
--- fs/quota/quota.c | 4 ++-- fs/sync.c | 20 +++++++++++--------- 2 files changed, 13 insertions(+), 11 deletions(-)
v3: reinstate wait/nowait variables for clarity
fs/quota/quota.c +2 -2
@@ -48,7 +48,7 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd,static void quota_sync_one (struct super_block *sb,void *arg) { -int type = *(int *)arg; +int type = (unsigned long )arg;if (sb->s_qcop && sb->s_qcop->quota_sync && (sb->s_quota_types & (1 << type)))
@@ -63,7 +63,7 @@ static int quota_sync_all(int type)return -EINVAL ; ret =security_quotactl (Q_SYNC , type,0 ,NULL );if (!ret) -iterate_supers (quota_sync_one, &type); +iterate_supers (quota_sync_one, (void *)((unsigned long )type));return ret; }
fs/sync.c +11 -9
@@ -76,8 +76,10 @@ static void sync_inodes_one_sb(struct super_block *sb, void *arg)static void sync_fs_one_sb (struct super_block *sb,void *arg) { +int wait = arg ?1 :0 ; +if (!sb_rdonly (sb) && sb->s_op->sync_fs) - sb->s_op->sync_fs (sb, *(int *)arg); + sb->s_op->sync_fs (sb, wait); }static void fdatawrite_one_bdev (struct block_device *bdev,void *arg)
@@ -107,12 +109,12 @@ static void fdatawait_one_bdev(struct block_device *bdev, void *arg)*/ void ksys_sync (void ) { -int nowait =0 , wait =1 ; +void *nowait =NULL , *wait = (void *)1UL ;wakeup_flusher_threads (WB_REASON_SYNC );iterate_supers (sync_inodes_one_sb,NULL ); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &wait); +iterate_supers (sync_fs_one_sb, nowait); +iterate_supers (sync_fs_one_sb, wait);iterate_bdevs (fdatawrite_one_bdev,NULL );iterate_bdevs (fdatawait_one_bdev,NULL );if (unlikely (laptop_mode))
@@ -127,17 +129,17 @@ SYSCALL_DEFINE0(sync)static void do_sync_work (struct work_struct *work) { -int nowait =0 ; +void *nowait =NULL ;/* * Sync twice to reduce the possibility we skipped some inodes / pages * because they were temporarily locked */ -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); +iterate_supers (sync_inodes_one_sb,NULL ); +iterate_supers (sync_fs_one_sb, nowait);iterate_bdevs (fdatawrite_one_bdev,NULL ); -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); +iterate_supers (sync_inodes_one_sb,NULL ); +iterate_supers (sync_fs_one_sb, nowait);iterate_bdevs (fdatawrite_one_bdev,NULL );printk ("Emergency Sync complete\n" );kfree (work);
Signature · 1 line
2.17.0
Re: [PATCH v3] vfs: avoid dereferencing pointers in iterate_supers callbacks
Quoted 11 lines · Jeff Layton
From: Jeff Layton <[email protected]>
All of the callback functions for
iterate_superseither ignore the opaque argument, or dereference the pointer only to fetch the int to which it points.Change
quota_sync_oneto just cast the int from the pointer, and changesync_fs_one_sbto just use a NULL/non-NULL pointer as a flag.Signed-off-by Jeff Layton <[email protected]>
As much as I have a feeling "why do we bother", the patch looks correct :). So feel free to add:
Honza
Quoted 88 lines
--- fs/quota/quota.c | 4 ++-- fs/sync.c | 20 +++++++++++--------- 2 files changed, 13 insertions(+), 11 deletions(-)v3: reinstate wait/nowait variables for clarity
fs/quota/quota.c +2
-2@@ -48,7 +48,7 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd,static void quota_sync_one (struct super_block *sb,void *arg) { -int type = *(int *)arg; +int type = (unsigned long )arg;if (sb->s_qcop && sb->s_qcop->quota_sync && (sb->s_quota_types & (1 << type)))@@ -63,7 +63,7 @@ static int quota_sync_all(int type)return -EINVAL ; ret =security_quotactl (Q_SYNC , type,0 ,NULL );if (!ret) -iterate_supers (quota_sync_one, &type); +iterate_supers (quota_sync_one, (void *)((unsigned long )type));return ret; }fs/sync.c +11
-9@@ -76,8 +76,10 @@ static void sync_inodes_one_sb(struct super_block *sb, void *arg)static void sync_fs_one_sb (struct super_block *sb,void *arg) { +int wait = arg ?1 :0 ; +if (!sb_rdonly (sb) && sb->s_op->sync_fs) - sb->s_op->sync_fs (sb, *(int *)arg); + sb->s_op->sync_fs (sb, wait); }static void fdatawrite_one_bdev (struct block_device *bdev,void *arg)@@ -107,12 +109,12 @@ static void fdatawait_one_bdev(struct block_device *bdev, void *arg)*/ void ksys_sync (void ) { -int nowait =0 , wait =1 ; +void *nowait =NULL , *wait = (void *)1UL ;wakeup_flusher_threads (WB_REASON_SYNC );iterate_supers (sync_inodes_one_sb,NULL ); -iterate_supers (sync_fs_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &wait); +iterate_supers (sync_fs_one_sb, nowait); +iterate_supers (sync_fs_one_sb, wait);iterate_bdevs (fdatawrite_one_bdev,NULL );iterate_bdevs (fdatawait_one_bdev,NULL );if (unlikely (laptop_mode))@@ -127,17 +129,17 @@ SYSCALL_DEFINE0(sync)static void do_sync_work (struct work_struct *work) { -int nowait =0 ; +void *nowait =NULL ;/* * Sync twice to reduce the possibility we skipped some inodes / pages * because they were temporarily locked */ -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); +iterate_supers (sync_inodes_one_sb,NULL ); +iterate_supers (sync_fs_one_sb, nowait);iterate_bdevs (fdatawrite_one_bdev,NULL ); -iterate_supers (sync_inodes_one_sb, &nowait); -iterate_supers (sync_fs_one_sb, &nowait); +iterate_supers (sync_inodes_one_sb,NULL ); +iterate_supers (sync_fs_one_sb, nowait);iterate_bdevs (fdatawrite_one_bdev,NULL );printk ("Emergency Sync complete\n" );kfree (work);Signature · 1 line
2.17.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR