From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:46875 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752238AbeERJc7 (ORCPT ); Fri, 18 May 2018 05:32:59 -0400 Date: Fri, 18 May 2018 11:32:57 +0200 From: Jan Kara To: Jeff Layton Cc: viro@zeniv.linux.org.uk, jack@suse.com, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, lurodriguez@suse.de, willy@infradead.org Subject: Re: [PATCH v3] vfs: avoid dereferencing pointers in iterate_supers callbacks Message-ID: <20180518093257.ug6idj2w4bpnxell@quack2.suse.cz> References: <20180517154646.18751-1-jlayton@kernel.org> <20180517215420.1810-1-jlayton@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180517215420.1810-1-jlayton@kernel.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu 17-05-18 17:54:20, Jeff Layton wrote: > From: Jeff Layton > > 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. > > Signed-off-by: Jeff Layton As much as I have a feeling "why do we bother", the patch looks correct :). So feel free to add: Reviewed-by: Jan Kara Honza > --- > fs/quota/quota.c | 4 ++-- > fs/sync.c | 20 +++++++++++--------- > 2 files changed, 13 insertions(+), 11 deletions(-) > > v3: reinstate wait/nowait variables for clarity > > diff --git a/fs/quota/quota.c b/fs/quota/quota.c > index 860bfbe7a07a..8dc76d5f87c7 100644 > --- a/fs/quota/quota.c > +++ b/fs/quota/quota.c > @@ -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; > } > > diff --git a/fs/sync.c b/fs/sync.c > index b54e0541ad89..a863cd2490ce 100644 > --- a/fs/sync.c > +++ b/fs/sync.c > @@ -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); > -- > 2.17.0 > > -- Jan Kara SUSE Labs, CR