Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Aug 2023 20:09:03 -0700
From:   "Darrick J. Wong" <djwong@kernel.org>
To:     Stephen Zhang <starzhangzsd@gmail.com>
Cc:     Andreas Dilger <adilger@dilger.ca>, Theodore Ts'o <tytso@mit.edu>,
        Zhang Yi <yi.zhang@huawei.com>,
        Ext4 Developers List <linux-ext4@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        zhangshida@kylinos.cn, stable@kernel.org
Subject: Re: [PATCH v3] ext4: Fix rec_len verify error
Message-ID: <20230803030903.GK11377@frogsfrogsfrogs>
References: <20230801112337.1856215-1-zhangshida@kylinos.cn>
 <20230801151828.GB11332@frogsfrogsfrogs>
 <A9ECDF14-95A1-4B1E-A815-4B6ABF4916C6@dilger.ca>
 <CANubcdUsDfiuGimNXjzoAF5ki8waCoFW31mg4vjpm073rS6+dw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CANubcdUsDfiuGimNXjzoAF5ki8waCoFW31mg4vjpm073rS6+dw@mail.gmail.com>
Precedence: bulk

On Thu, Aug 03, 2023 at 09:52:53AM +0800, Stephen Zhang wrote:
> Andreas Dilger <adilger@dilger.ca> 于2023年8月2日周三 14:07写道：
> >
> > Not all of these cases are actual bugs.  The ext4_rec_len_from_disk()
> > function is only different for rec_len >= 2^16, so if it is comparing
> > rec_len against "12" or "sizeof(struct ...)" then the inequality will
> > be correct regardless of how it is decoded.
> >
> > That said, it makes sense to use ext4_rec_len_from_disk() to access
> > rec_len consistently throughout the code, since that avoids potential
> > bugs in the future.  We know the code will eventually will be copied
> > some place where rec_len >= 2^16 is actually important, and we may as
> > well avoid that bug before it happens.
> >
> >
> > One thing this discussion *does* expose is that ext4_rec_len_from_disk()
> > is hard-coded at compile time to differentiate between PAGE_SIZE > 64k
> > and PAGE_SIZE = 4K, because it was never possible to have blocksize >
> > PAGE_SIZE, so only ARM/PPC ever had filesystems with blocksize=64KiB
> > (and the Fujitsu Fugaku SPARC system with blocksize=256KiB).
> >
> > However, with the recent advent of the VM and IO layers allowing
> > blocksize > PAGE_SIZE this function will need to be changed to allow
> > the same on x86 PAGE_SIZE=4KiB systems.  Instead of checking
> >
> >   #if PAGE_SIZE >= 65536
> >
> > it should handle this based on the filesystem blocksize at runtime:
> >
> > static inline
> > unsigned int ext4_rec_len_from_disk(__le16 dlen, unsigned blocksize)
> > {
> >         unsigned len = le16_to_cpu(dlen);
> >
> >         if (blocksize < 65536)
> >                 return len;
> >
> >         if (len == EXT4_MAX_REC_LEN || len == 0)
> >                 return blocksize;
> >
> >         return (len & 65532) | ((len & 3) << 16);
> > }
> >
> > Strictly speaking, ((len & 65532) | ((len & 3) << 16) should equal "len"
> > for any filesystem with blocksize < 65536, but IMHO it is more clear if
> > the code is written this way.
> >
> > Similarly, the encoding needs to be changed to handle large records at
> > runtime for when we eventually allow ext4 with blocksize > PAGE_SIZE.
> >
> > static inline __le16 ext4_rec_len_to_disk(unsigned len, unsigned blocksize)
> > {
> >         BUG_ON(len > blocksize);
> >         BUG_ON(blocksize > (1 << 18));
> >         BUG_ON(len & 3);
> >
> >         if (len < 65536) /* always true for blocksize < 65536 */
> >                 return cpu_to_le16(len);
> >
> >         if (len == blocksize) {
> >                 if (blocksize == 65536)
> >                         return cpu_to_le16(EXT4_MAX_REC_LEN);
> >
> >                 return cpu_to_le16(0);
> >         }
> >
> >         return cpu_to_le16((len & 65532) | ((len >> 16) & 3));
> > }
> >
> 
> Hmm, at least it sounds reasonable to me based on my limited
> knowledge. However, I am not sure whether you want me to incorporate
> these changes into this particular commit or another patch within this
> submission.
> 
> By default, I will simply leave it for further discussion. Please let
> me know if you have any ideas.

ext4 doesn't support blocksize > PAGE_SIZE yet.  Don't worry about this
for now.

--D

> Cheers,
> Shida
> 
> >
> > Cheers, Andreas
> >
> >
> >
> >
> >