Re: [PATCH 2/5] mm/mseal: move madvise() logic to mm/madvise.c

[David Hildenbrand <david@redhat.com>]

On 14.07.25 17:18, Lorenzo Stoakes wrote:
> On Mon, Jul 14, 2025 at 05:03:03PM +0200, David Hildenbrand wrote:
>>>> or sth like that would surely clean that up further.
>>>
>>> Well, I plan to make this not a thing soon so I'd rather not.
>>>
>>> The intent is to make _all_ VMA flags work on 32-bit kernels. I have done some
>>> preparatory work and next cycle intend to do more on this.
>>>
>>> So I'd rather avoid any config changes on this until I've given this a shot.
>>
>> Sure, if that is in sight.
> 
> Yes :)
> 
>>>>> + * only do so via an appropriate madvise() call.
>>>>> + */
>>>>> +static bool can_madvise_modify(struct madvise_behavior *madv_behavior)
>>>>> +{
>>>>> +	struct vm_area_struct *vma = madv_behavior->vma;
>>>>> +
>>>>> +	/* If the operation won't discard, we're good. */
>>>>> +	if (!is_discard(madv_behavior->behavior))
>>>>> +		return true;
>>>>
>>>>
>>>> Conceptually, I would do this first and then handle all the discard cases /
>>>> exceptions.
>>>
>>> Hm I'm confused :P we do do this first? I think the idea with this is we can
>>> very cheaply ignore any MADV_ that isn't applicable.
>>>
>>> Did you mean to put this comment under line below?
>>>
>>> I mean it's not exactly a perf hotspot so don't mind moving them around.
>>
>> I was thinking of this (start with sealed, then go into details about
>> discards):
>>
>> /* If the VMA isn't sealed, we're all good. */
>> if (can_modify_vma(vma))
>> 	return true;
>>
>> /* In a sealed VMA, we only care about discard operations. */
>> if (!is_discard(madv_behavior->behavior))
>> 	return true;
>>
>> /* But discards of file-backed mappings are fine. */
>> if (!vma_is_anonymous(vma))
>> 	return true;
> 
> Right yeah.
> 
>>
>> ...
>>
>>
>> But now I wonder, why is it okay to discard anon pages in a MAP_PRIVATE file
>> mapping?
> 
> I'm duplicating existing logic here (well updating from the vma->vm_file check
> and a seemingly pointless !vma->vm_file && vma->vm_flags & VM_SHARED check), but
> this is a good point...

Yeah, not blaming you, just scratching my head :)

> 
> For the purposes of the refactoring I guess best to keep the logic ostensibly
> the same given the 'no functional change intended', but we do need to fix this
> yes.


Likely a fix should be pulled in early? Not sure ... but it sure sounds 
broken.

-- 
Cheers,

David / dhildenb