On Fri, Mar 21, 2025 at 11:26:18AM +0000, Pedro Falcato wrote: > On Fri, Mar 21, 2025 at 10:09:37AM +0000, Lorenzo Stoakes wrote: > > Currently, if a VMA merge fails due to an OOM condition arising on commit > > merge or a failure to duplicate anon_vma's, we report this so the caller > > can handle it. > > > > However there are cases where the caller is only ostensibly trying a > > merge, and doesn't mind if it fails due to this condition. > > > > Ok, so here's my problem with your idea: I don't think merge should be exposed > to vma_modify() callers. Right now (at least AIUI), you want to modify a given > VMA, you call vma_modify(), and it gives you a vma you can straight up modify > without any problems. Essentially breaks down any VMAs necessary. This feels > contractually simple and easy to use, and I don't think leaking details about > merging is the correct approach here. The vmg passed is already 'exposing' merge and has flags you can change. There's no contract that vma_modify() abstracts this, you're saying you want to modify, maybe merge if we can. Uffd is actually calling it in a purely special case - one where you would never split. I mean an alternative is to have something not-vma_modify() do it, but then we end up with code duplication, which is why I made the unregister + clear paths do the same thing here. > > > Since we do not want to introduce an implicit assumption that we only > > actually modify VMAs after OOM conditions might arise, add a 'give up on > > oom' option and make an explicit contract that, should this flag be set, we > > absolutely will not modify any VMAs should OOM arise and just bail out. > > > > Thus, to me the most natural solution is still mine. Do you think it places too > many constraints on vma_modify()? vma_modify() on a single VMA, without > splitting, Just Working(tm) is a sensible expectation (and vma_merge being fully > best-effort). Things like mprotect() failing due to OOM are also pretty disastrous, > so if we could limit that it'd be great. I disagree, again for the same reason as stated before, you are making an implicit assumption that an OOM error means the VMA is not deleted. This only happens to be true _now_. Having this implicit assumption there, which later might change, is _precisely_ the kind of thing that led us to this issue in the first place. So that's just not workable. A version of your thing that would work is where vma_modify() itself sets the flag so we -establish this contract-. But I don't want to infect all other callers who don't have uffd's problem with this. Also, again, this is a -uffd- problem. Uffd is calling a function that can return an error, assuming it must not return an error, and breaking if it does. We have to on some level have uffd say 'actually we rely on this'. So, ugly as it is, I feel my approach is the best for now. But to be revisited! > > In any case, your solution looks palatable to me, but I want to make > sure we're not making this excessively complicated. Thanks! I don't think this is any more complicated than it needs to be, as Liam alludes to in his reply. But rethinking this whole thing on a deeper level is on my agenda now. Error paths are an ugly pain anywwhere :( > > -- > Pedro
