| 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
| ||
|
Message-ID: <aS1FVIfE0Ntgbr5I@infradead.org>
Date: Sun, 30 Nov 2025 23:35:48 -0800
From: Christoph Hellwig <hch@...radead.org>
To: Namjae Jeon <linkinjeon@...nel.org>
Cc: viro@...iv.linux.org.uk, brauner@...nel.org, hch@...radead.org,
hch@....de, tytso@....edu, willy@...radead.org, jack@...e.cz,
djwong@...nel.org, josef@...icpanda.com, sandeen@...deen.net,
rgoldwyn@...e.com, xiang@...nel.org, dsterba@...e.com,
pali@...nel.org, ebiggers@...nel.org, neil@...wn.name,
amir73il@...il.com, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, iamjoonsoo.kim@....com,
cheol.lee@....com, jay.sim@....com, gunho.lee@....com,
Hyunchul Lee <hyc.lee@...il.com>
Subject: Re: [PATCH v2 06/11] ntfsplus: add iomap and address space operations
> +#include "ntfs_iomap.h"
> +
> +static s64 ntfs_convert_page_index_into_lcn(struct ntfs_volume *vol, struct ntfs_inode *ni,
> + unsigned long page_index)
> +{
> + sector_t iblock;
> + s64 vcn;
> + s64 lcn;
> + unsigned char blocksize_bits = vol->sb->s_blocksize_bits;
> +
> + iblock = (s64)page_index << (PAGE_SHIFT - blocksize_bits);
> + vcn = (s64)iblock << blocksize_bits >> vol->cluster_size_bits;
I've seen this calculate in quite a few places, should there be a
generic helper for it?
> +struct bio *ntfs_setup_bio(struct ntfs_volume *vol, blk_opf_t opf, s64 lcn,
> + unsigned int pg_ofs)
> +{
> + struct bio *bio;
> +
> + bio = bio_alloc(vol->sb->s_bdev, 1, opf, GFP_NOIO);
> + if (!bio)
> + return NULL;
bio_alloc never returns NULL if it can sleep.
> + bio->bi_iter.bi_sector = ((lcn << vol->cluster_size_bits) + pg_ofs) >>
> + vol->sb->s_blocksize_bits;
With a helper to calculate the sector the ntfs_setup_bio helper becomes
somewhat questionable.
> +static int ntfs_read_folio(struct file *file, struct folio *folio)
> +{
> + loff_t i_size;
> + struct inode *vi;
> + struct ntfs_inode *ni;
> +
> + vi = folio->mapping->host;
> + i_size = i_size_read(vi);
> + /* Is the page fully outside i_size? (truncate in progress) */
> + if (unlikely(folio->index >= (i_size + PAGE_SIZE - 1) >>
> + PAGE_SHIFT)) {
> + folio_zero_segment(folio, 0, PAGE_SIZE);
> + ntfs_debug("Read outside i_size - truncated?");
> + folio_mark_uptodate(folio);
> + folio_unlock(folio);
> + return 0;
> + }
iomap should be taking care of this, why do you need the extra
handling?
> + /*
> + * This can potentially happen because we clear PageUptodate() during
> + * ntfs_writepage() of MstProtected() attributes.
> + */
> + if (folio_test_uptodate(folio)) {
> + folio_unlock(folio);
> + return 0;
> + }
Clearing the folio uptodate flag sounds fairly dangerous, why is that
done?
> +static int ntfs_write_mft_block(struct ntfs_inode *ni, struct folio *folio,
> + struct writeback_control *wbc)
Just a very high-level comment here with no immediate action needed:
Is there a reall good reason to use the page cache for metadata?
Our experience with XFS is that a dedicated buffer cache is not only
much easier to use, but also allows for much better caching.
> +static void ntfs_readahead(struct readahead_control *rac)
> +{
> + struct address_space *mapping = rac->mapping;
> + struct inode *inode = mapping->host;
> + struct ntfs_inode *ni = NTFS_I(inode);
> +
> + if (!NInoNonResident(ni) || NInoCompressed(ni)) {
> + /* No readahead for resident and compressed. */
> + return;
> + }
> +
> + if (NInoMstProtected(ni) &&
> + (ni->mft_no == FILE_MFT || ni->mft_no == FILE_MFTMirr))
> + return;
Can you comment on why readahead is skipped here?
> +/**
> + * ntfs_compressed_aops - address space operations for compressed inodes
> + */
> +const struct address_space_operations ntfs_compressed_aops = {
>From code in other patches is looks like ntfs never switches between
compressed and non-compressed for live inodes? In that case the
separate aops should be fine, as switching between them at runtime
would involve races. Is the compression policy per-directory?
> + kaddr = kmap_local_folio(folio, 0);
> + offset = (loff_t)idx << PAGE_SHIFT;
> + to = min_t(u32, end - offset, PAGE_SIZE);
> +
> + memcpy(buf + buf_off, kaddr + from, to);
> + buf_off += to;
> + kunmap_local(kaddr);
> + folio_put(folio);
> + }
Would this be a candidate for memcpy_from_folio?
> + kaddr = kmap_local_folio(folio, 0);
> + offset = (loff_t)idx << PAGE_SHIFT;
> + to = min_t(u32, end - offset, PAGE_SIZE);
> +
> + memcpy(kaddr + from, buf + buf_off, to);
> + buf_off += to;
> + kunmap_local(kaddr);
> + folio_mark_uptodate(folio);
> + folio_mark_dirty(folio);
And memcpy_to_folio?
> +++ b/fs/ntfsplus/ntfs_iomap.c
Any reason for the ntfs_ prefix here?
> +static void ntfs_iomap_put_folio(struct inode *inode, loff_t pos,
> + unsigned int len, struct folio *folio)
> +{
This seems to basically be entirely about extra zeroing. Can you
explain why this is needed in a comment?
> +static int ntfs_read_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> + unsigned int flags, struct iomap *iomap, struct iomap *srcmap)
> +{
> + struct ntfs_inode *base_ni, *ni = NTFS_I(inode);
> + struct ntfs_attr_search_ctx *ctx;
> + loff_t i_size;
> + u32 attr_len;
> + int err = 0;
> + char *kattr;
> + struct page *ipage;
> +
> + if (NInoNonResident(ni)) {
Can you split the resident and non-resident cases into separate
helpers to keep this easier to follow?
easier to follow?
> + ipage = alloc_page(__GFP_NOWARN | __GFP_IO | __GFP_ZERO);
> + if (!ipage) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + memcpy(page_address(ipage), kattr, attr_len);
Is there a reason for this being a page allocation vs a kmalloc
sized to the inline data?
> +static int ntfs_buffered_zeroed_clusters(struct inode *vi, s64 vcn)
I think this should be ntfs_buffered_zero_clusters as it
performans the action?
Also curious why this can't use the existing iomap zeroing helper?
> +int ntfs_zeroed_clusters(struct inode *vi, s64 lcn, s64 num)
ntfs_zero_clusters
Again curious why we need special zeroing code in the file system.
> + if (NInoNonResident(ni)) {
Another case for splitting the resident/non-resident code instead
of having a giant conditional block that just returns.
Powered by blists - more mailing lists