On Wed, Feb 26, 2025 at 07:01:36PM +0100, Oleg Nesterov wrote: > On 02/26, Lorenzo Stoakes wrote: > > > > On Wed, Feb 26, 2025 at 05:26:04PM +0100, Oleg Nesterov wrote: > > > On 02/24, jeffxu@xxxxxxxxxxxx wrote: > > > > > > > > Unlike other system mappings, the uprobe mapping is not > > > > established during program startup. However, its lifetime is the same > > > > as the process's lifetime. It could be sealed from creation. > > > > > > Agreed, VM_SEALED should be always for the "[uprobes]" vma, regardless > > > of config options. > > > > If you think this ought to be the case generally, then perhaps we should > > drop this patch from the commit and just do this separately as a > > permanent-on thing, if you are sure this is fine + want it? > > See below... > > > An aside - we _definitely_ cannot allow this -system mapping stuff- to be > > enabled without a config option, > > This is clear. > > But as for uprobes in particular I do think that VM_SEALED is always fine. > > Do we really want it? I dunno. If a task unmaps its "[uprobes]" vma it > will crash when it hits the uprobes bp next time. Unless the probed insn > can be emulated and it is not ret-probe. Do we really care? Again, I don't > know. > > Should this change come as a separate patch? I don't understand why it should > but I am fine either way. > > In short. please do what you think is right, VM_SEALED can't hurt uprobes ;) > > > > #ifdef CONFIG_64BIT > > > /* VM is sealed, in vm_flags */ > > > #define VM_SEALED _BITUL(63) > > > + #else > > > + #define VM_SEALED 0 > > > #endif > > > > This has been raised a few times. Jeff objects to this > > OK, > > > > and then simply > > > > > > vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE, > > > - VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO, > > > + VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED, > > > > > > ? > > > > Nah you'd have to do: > > > > > > > vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE, > > VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO > > #ifdef CONFIG_64BIT > > VM_SEALED > > #endif > > , > > Why??? With the proposed change above VM_SEALED == 0 if !CONFIG_64BIT. > Like I said, Jeff opposes the change. I disagree with him, and agree with you, because this is very silly. But I don't want to hold up this series with that discussion (this is for his sake...) > Oleg. > Jeff - perhaps drop this and let's return to it in a follow up so this series isn't held up? Thanks.
