| 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: <b67dd7cd-2c1c-4566-badf-32082d8cd952@redhat.com>
Date: Thu, 25 Sep 2025 13:55:26 +0200
From: David Hildenbrand <david@...hat.com>
To: "Garg, Shivank" <shivankg@....com>,
Sean Christopherson <seanjc@...gle.com>,
Ackerley Tng <ackerleytng@...gle.com>
Cc: willy@...radead.org, akpm@...ux-foundation.org, pbonzini@...hat.com,
shuah@...nel.org, vbabka@...e.cz, brauner@...nel.org,
viro@...iv.linux.org.uk, dsterba@...e.com, xiang@...nel.org,
chao@...nel.org, jaegeuk@...nel.org, clm@...com, josef@...icpanda.com,
kent.overstreet@...ux.dev, zbestahu@...il.com, jefflexu@...ux.alibaba.com,
dhavale@...gle.com, lihongbo22@...wei.com, lorenzo.stoakes@...cle.com,
Liam.Howlett@...cle.com, rppt@...nel.org, surenb@...gle.com,
mhocko@...e.com, ziy@...dia.com, matthew.brost@...el.com,
joshua.hahnjy@...il.com, rakie.kim@...com, byungchul@...com,
gourry@...rry.net, ying.huang@...ux.alibaba.com, apopple@...dia.com,
tabba@...gle.com, paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
pvorel@...e.cz, bfoster@...hat.com, vannapurve@...gle.com,
chao.gao@...el.com, bharata@....com, nikunj@....com, michael.day@....com,
shdhiman@....com, yan.y.zhao@...el.com, Neeraj.Upadhyay@....com,
thomas.lendacky@....com, michael.roth@....com, aik@....com, jgg@...dia.com,
kalyazin@...zon.com, peterx@...hat.com, jack@...e.cz, hch@...radead.org,
cgzones@...glemail.com, ira.weiny@...el.com, rientjes@...gle.com,
roypat@...zon.co.uk, chao.p.peng@...el.com, amit@...radead.org,
ddutile@...hat.com, dan.j.williams@...el.com, ashish.kalra@....com,
gshan@...hat.com, jgowans@...zon.com, pankaj.gupta@....com,
papaluri@....com, yuzhao@...gle.com, suzuki.poulose@....com,
quic_eberman@...cinc.com, linux-bcachefs@...r.kernel.org,
linux-btrfs@...r.kernel.org, linux-erofs@...ts.ozlabs.org,
linux-f2fs-devel@...ts.sourceforge.net, linux-fsdevel@...r.kernel.org,
linux-mm@...ck.org, linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org, kvm@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-coco@...ts.linux.dev
Subject: Re: [PATCH kvm-next V11 4/7] KVM: guest_memfd: Use guest mem inodes
instead of anonymous inodes
On 25.09.25 13:44, Garg, Shivank wrote:
>
>
> On 9/25/2025 8:20 AM, Sean Christopherson wrote:
>> My apologies for the super late feedback. None of this is critical (mechanical
>> things that can be cleaned up after the fact), so if there's any urgency to
>> getting this series into 6.18, just ignore it.
>>
>> On Wed, Aug 27, 2025, Ackerley Tng wrote:
>>> Shivank Garg <shivankg@....com> writes:
>>> @@ -463,11 +502,70 @@ bool __weak kvm_arch_supports_gmem_mmap(struct kvm *kvm)
>>> return true;
>>> }
>>>
>>> +static struct inode *kvm_gmem_inode_create(const char *name, loff_t size,
>>> + u64 flags)
>>> +{
>>> + struct inode *inode;
>>> +
>>> + inode = anon_inode_make_secure_inode(kvm_gmem_mnt->mnt_sb, name, NULL);
>>> + if (IS_ERR(inode))
>>> + return inode;
>>> +
>>> + inode->i_private = (void *)(unsigned long)flags;
>>> + inode->i_op = &kvm_gmem_iops;
>>> + inode->i_mapping->a_ops = &kvm_gmem_aops;
>>> + inode->i_mode |= S_IFREG;
>>> + inode->i_size = size;
>>> + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
>>> + mapping_set_inaccessible(inode->i_mapping);
>>> + /* Unmovable mappings are supposed to be marked unevictable as well. */
>>> + WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
>>> +
>>> + return inode;
>>> +}
>>> +
>>> +static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,
>>> + u64 flags)
>>> +{
>>> + static const char *name = "[kvm-gmem]";
>>> + struct inode *inode;
>>> + struct file *file;
>>> + int err;
>>> +
>>> + err = -ENOENT;
>>> + /* __fput() will take care of fops_put(). */
>>> + if (!fops_get(&kvm_gmem_fops))
>>> + goto err;
>>> +
>>> + inode = kvm_gmem_inode_create(name, size, flags);
>>> + if (IS_ERR(inode)) {
>>> + err = PTR_ERR(inode);
>>> + goto err_fops_put;
>>> + }
>>> +
>>> + file = alloc_file_pseudo(inode, kvm_gmem_mnt, name, O_RDWR,
>>> + &kvm_gmem_fops);
>>> + if (IS_ERR(file)) {
>>> + err = PTR_ERR(file);
>>> + goto err_put_inode;
>>> + }
>>> +
>>> + file->f_flags |= O_LARGEFILE;
>>> + file->private_data = priv;
>>> +
>>> + return file;
>>> +
>>> +err_put_inode:
>>> + iput(inode);
>>> +err_fops_put:
>>> + fops_put(&kvm_gmem_fops);
>>> +err:
>>> + return ERR_PTR(err);
>>> +}
>>
>> I don't see any reason to add two helpers. It requires quite a bit more lines
>> of code due to adding more error paths and local variables, and IMO doesn't make
>> the code any easier to read.
>>
>> Passing in "gmem" as @priv is especially ridiculous, as it adds code and
>> obfuscates what file->private_data is set to.
>>
>> I get the sense that the code was written to be a "replacement" for common APIs,
>> but that is nonsensical (no pun intended).
>>
>>> static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
>>> {
>>> - const char *anon_name = "[kvm-gmem]";
>>> struct kvm_gmem *gmem;
>>> - struct inode *inode;
>>> struct file *file;
>>> int fd, err;
>>>
>>> @@ -481,32 +579,16 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
>>> goto err_fd;
>>> }
>>>
>>> - file = anon_inode_create_getfile(anon_name, &kvm_gmem_fops, gmem,
>>> - O_RDWR, NULL);
>>> + file = kvm_gmem_inode_create_getfile(gmem, size, flags);
>>> if (IS_ERR(file)) {
>>> err = PTR_ERR(file);
>>> goto err_gmem;
>>> }
>>>
>>> - file->f_flags |= O_LARGEFILE;
>>> -
>>> - inode = file->f_inode;
>>> - WARN_ON(file->f_mapping != inode->i_mapping);
>>> -
>>> - inode->i_private = (void *)(unsigned long)flags;
>>> - inode->i_op = &kvm_gmem_iops;
>>> - inode->i_mapping->a_ops = &kvm_gmem_aops;
>>> - inode->i_mode |= S_IFREG;
>>> - inode->i_size = size;
>>> - mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
>>> - mapping_set_inaccessible(inode->i_mapping);
>>> - /* Unmovable mappings are supposed to be marked unevictable as well. */
>>> - WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
>>> -
>>> kvm_get_kvm(kvm);
>>> gmem->kvm = kvm;
>>> xa_init(&gmem->bindings);
>>> - list_add(&gmem->entry, &inode->i_mapping->i_private_list);
>>> + list_add(&gmem->entry, &file_inode(file)->i_mapping->i_private_list);
>>
>> I don't understand this change? Isn't file_inode(file) == inode?
>>
>> Compile tested only, and again not critical, but it's -40 LoC...
>>
>>
>
> Thanks.
> I did functional testing and it works fine.
I can queue this instead. I guess I can reuse the patch description and
add Sean as author + add his SOB (if he agrees).
Let me take a look at the patch later in more detail.
--
Cheers
David / dhildenb
Powered by blists - more mailing lists