linux-kernel - Re: [PATCH v8 06/10] iomap: fix iomap_dio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240702140123.emt2gz5kbigth2en@quentin>
Date: Tue, 2 Jul 2024 14:01:23 +0000
From: "Pankaj Raghav (Samsung)" <kernel@...kajraghav.com>
To: Christoph Hellwig <hch@....de>
Cc: david@...morbit.com, willy@...radead.org, chandan.babu@...cle.com,
	djwong@...nel.org, brauner@...nel.org, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, yang@...amperecomputing.com,
	linux-mm@...ck.org, john.g.garry@...cle.com,
	linux-fsdevel@...r.kernel.org, hare@...e.de, p.raghav@...sung.com,
	mcgrof@...nel.org, gost.dev@...sung.com, cl@...amperecomputing.com,
	linux-xfs@...r.kernel.org, Zi Yan <zi.yan@...t.com>
Subject: Re: [PATCH v8 06/10] iomap: fix iomap_dio_zero() for fs bs > system
 page size

> > > A WARN_ON without actually erroring out here is highly dangerous. 
> > 
> > I agree but I think we decided that we are safe with 64k for now as fs 
> > that uses iomap will not have a block size > 64k. 
> > 
> > But this function needs some changes when we decide to go beyond 64k
> > by returning error instead of not returning anything. 
> > Until then WARN_ON_ONCE would be a good stop gap for people developing
> > the feature to go beyond 64k block size[1]. 
> 
> Sure, but please make it return an error and return that instead of
> just warning and going beyond the allocated page.

Does this make sense?

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 61d09d2364f7..14be34703588 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -240,16 +240,19 @@ void iomap_dio_bio_end_io(struct bio *bio)
 }
 EXPORT_SYMBOL_GPL(iomap_dio_bio_end_io);
 
-static void iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
+static int iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
                loff_t pos, unsigned len)
 {
        struct inode *inode = file_inode(dio->iocb->ki_filp);
        struct bio *bio;
 
+       if (!len)
+               return 0;
        /*
         * Max block size supported is 64k
         */
-       WARN_ON_ONCE(len > ZERO_PAGE_64K_SIZE);
+       if (len > ZERO_PAGE_64K_SIZE)
+               return -EINVAL;
 
        bio = iomap_dio_alloc_bio(iter, dio, 1, REQ_OP_WRITE | REQ_SYNC | REQ_IDLE);
        fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
@@ -260,6 +263,7 @@ static void iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
 
        __bio_add_page(bio, zero_page_64k, len, 0);
        iomap_dio_submit_bio(iter, dio, bio, pos);
+       return 0;
 }
 
 /*
@@ -368,8 +372,10 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
        if (need_zeroout) {
                /* zero out from the start of the block to the write offset */
                pad = pos & (fs_block_size - 1);
-               if (pad)
-                       iomap_dio_zero(iter, dio, pos - pad, pad);
+
+               ret = iomap_dio_zero(iter, dio, pos - pad, pad);
+               if (ret)
+                       goto out;
        }
 
        /*
@@ -443,7 +449,7 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
                /* zero out from the end of the write to the end of the block */
                pad = pos & (fs_block_size - 1);
                if (pad)
-                       iomap_dio_zero(iter, dio, pos, fs_block_size - pad);
+                       ret = iomap_dio_zero(iter, dio, pos, fs_block_size - pad);
        }
 out:
        /* Undo iter limitation to current extent */

--
Pankaj