On Thu, 21 May 2026 at 13:59, Sean Christopherson <[email protected]> wrote:
>
> On Thu, May 21, 2026, Fuad Tabba wrote:
> > Hi Ackerley,
> >
> > On Thu, 7 May 2026 at 21:22, Ackerley Tng via B4 Relay
> > <[email protected]> wrote:
> > >
> > > From: Ackerley Tng <[email protected]>
> > >
> > > __kvm_gmem_invalidate_begin() and __kvm_gmem_invalidate_end() actually do
> > > not specially handle -1ul. -1ul is used as a huge number, which legal
> > > indices do not exceed, and hence the invalidation works as expected.
> > >
> > > Since a later patch is going to make use of the exact range, calculate the
> > > size of the guest_memfd inode and use it as the end range for invalidating
> > > SPTEs.
> > >
> > > Signed-off-by: Ackerley Tng <[email protected]>
> >
> > Want to look at what Sashiko has to say? Seems to be a real issue:
> >
> > https://sashiko.dev/#/patchset/20260507-gmem-inplace-conversion-v6-0-91ab5a8b19a4%40google.com?part=16
> >
> > If I understand correctly, the fix should simple: use
> > check_add_overflow() to validate the offset and size parameters in
> > kvm_gmem_bind()
> >
> > int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> > unsigned int fd, loff_t offset)
> > {
> > loff_t size = slot->npages << PAGE_SHIFT;
> > + loff_t end;
> > unsigned long start, end_index;
> > struct gmem_file *f;
> > ...
> > - if (offset < 0 || !PAGE_ALIGNED(offset) ||
> > - offset + size > i_size_read(inode))
> > + if (offset < 0 || !PAGE_ALIGNED(offset) ||
> > + check_add_overflow(offset, size, &end) ||
>
> Eww, TIL I'm not a fan of check_add_overflow(). Burying an out-param in an
> if-statement is nasty.
>
> > + end > i_size_read(inode))
>
> This is all rather silly. @offset and and @slot->npages are fundamentally
> unsigned values. I don't see any reason to convert them to signed values,
> only
> to convert them *back* to unsigned values (when stored in start/end, because
> xarrays
> operate on "unsigned long" indices).
>
> i_size_read() obviously has to return a positive value, so can't we just do
> this?
lgtm,
/fuad
>
> diff --git virt/kvm/guest_memfd.c virt/kvm/guest_memfd.c
> index a35a55571a2d..9c6dbb54e800 100644
> --- virt/kvm/guest_memfd.c
> +++ virt/kvm/guest_memfd.c
> @@ -640,9 +640,9 @@ int kvm_gmem_create(struct kvm *kvm, struct
> kvm_create_guest_memfd *args)
> }
>
> int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> - unsigned int fd, loff_t offset)
> + unsigned int fd, u64 offset)
> {
> - loff_t size = slot->npages << PAGE_SHIFT;
> + u64 size = slot->npages << PAGE_SHIFT;
> unsigned long start, end;
> struct gmem_file *f;
> struct inode *inode;
> @@ -664,8 +664,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot
> *slot,
>
> inode = file_inode(file);
>
> - if (offset < 0 || !PAGE_ALIGNED(offset) ||
> - offset + size > i_size_read(inode))
> + if (!PAGE_ALIGNED(offset) || offset + size > i_size_read(inode))
> goto err;
>
> filemap_invalidate_lock(inode->i_mapping);
> diff --git virt/kvm/kvm_mm.h virt/kvm/kvm_mm.h
> index 9fcc5d5b7f8d..3cb5ef86d0d9 100644
> --- virt/kvm/kvm_mm.h
> +++ virt/kvm/kvm_mm.h
> @@ -72,7 +72,7 @@ int kvm_gmem_init(struct module *module);
> void kvm_gmem_exit(void);
> int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args);
> int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> - unsigned int fd, loff_t offset);
> + unsigned int fd, u64 offset);
> void kvm_gmem_unbind(struct kvm_memory_slot *slot);
> #else
> static inline int kvm_gmem_init(struct module *module)
> @@ -80,9 +80,8 @@ static inline int kvm_gmem_init(struct module *module)
> return 0;
> }
> static inline void kvm_gmem_exit(void) {};
> -static inline int kvm_gmem_bind(struct kvm *kvm,
> - struct kvm_memory_slot *slot,
> - unsigned int fd, loff_t offset)
> +static inline int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot
> *slot,
> + unsigned int fd, u64 offset)
> {
> WARN_ON_ONCE(1);
> return -EIO;
>
