* Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> [250226 13:06]: > 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 nit, we have VM_NONE for this (it's also 0, so no real difference) > > > > #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. Discussion here [1]. > > 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, Liam [1]. https://lore.kernel.org/all/CABi2SkVKhjShryG0K-NSjjBvGs0UOVsY-6MQVOuQCkfuph5K8Q@xxxxxxxxxxxxxx/
