diff mbox

[v2] vfs: avoid dereferencing pointers in iterate_supers callbacks

Message ID 20180517174336.18529-1-jlayton@kernel.org (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Layton May 17, 2018, 5:43 p.m. UTC 
From: Jeff Layton <jlayton@redhat.com>

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 <jlayton@redhat.com>
---
 fs/quota/quota.c |  4 ++--
 fs/sync.c        | 20 +++++++++-----------
 2 files changed, 11 insertions(+), 13 deletions(-)

Comments

Matthew Wilcox May 17, 2018, 6:26 p.m. UTC | #1 
On Thu, May 17, 2018 at 01:43:36PM -0400, Jeff Layton wrote:
> @@ -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);
Jeff Layton May 17, 2018, 6:55 p.m. UTC | #2 
On Thu, 2018-05-17 at 11:26 -0700, Matthew Wilcox wrote:
> On Thu, May 17, 2018 at 01:43:36PM -0400, Jeff Layton wrote:
> > @@ -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);
> 

Sure, I could do that. We could also keep the wait/nowait vars too, I
guess.
diff mbox

Patch

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..d7330bb97f05 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,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);