On Mon, Jan 13, 2025 at 12:42:53PM -0800, Suren Baghdasaryan wrote: > On Mon, Jan 13, 2025 at 12:32 PM Vlastimil Babka <vbabka@xxxxxxx> wrote: > > > > On 1/13/25 20:11, Suren Baghdasaryan wrote: > > > On Mon, Jan 13, 2025 at 9:13 AM Lorenzo Stoakes > > > <lorenzo.stoakes@xxxxxxxxxx> wrote: > > >> > > >> On Mon, Jan 13, 2025 at 09:02:50AM -0800, Suren Baghdasaryan wrote: > > >> > On Mon, Jan 13, 2025 at 4:05 AM Lorenzo Stoakes > > >> > <lorenzo.stoakes@xxxxxxxxxx> wrote: > > >> > > > > >> > > On Fri, Jan 10, 2025 at 08:25:52PM -0800, Suren Baghdasaryan wrote: > > >> > > > When exit_mmap() removes vmas belonging to an exiting task, it does not > > >> > > > mark them as detached since they can't be reached by other tasks and they > > >> > > > will be freed shortly. Once we introduce vma reuse, all vmas will have to > > >> > > > be in detached state before they are freed to ensure vma when reused is > > >> > > > in a consistent state. Add missing vma_mark_detached() before freeing the > > >> > > > vma. > > >> > > > > >> > > Hmm this really makes me worry that we'll see bugs from this detached > > >> > > stuff, do we make this assumption anywhere else I wonder? > > >> > > > >> > This is the only place which does not currently detach the vma before > > >> > freeing it. If someone tries adding a case like that in the future, > > >> > they will be met with vma_assert_detached() inside vm_area_free(). > > >> > > >> OK good to know! > > >> > > >> Again, I wonder if we should make these assertions stronger as commented > > >> elsewhere, because if we see them in production isn't that worth an actual > > >> non-debug WARN_ON_ONCE()? > > > > > > Sure. I'll change vma_assert_attached()/vma_assert_detached() to use > > > WARN_ON_ONCE() and to return a bool (see also my reply in the patch > > > [0/17]). > > > > So is this a case of "someone might introduce code later that will violate > > them" as alluded to above? Unconditional WARN_ON_ONCE seems too much then. My concern is that there is a broken case that remains hidden because nothing is actually checked in production, which would then become really difficult to debug should somebody report it. We intend the WARN_ONxxx() functions to be asserting things that -should not be- for precisely this kind of purpose so I think it makes sense here. > > Yes, I wanted to make sure refcounting will not be broken by someone > doing re-attach/re-detach. Yes, and debugging this without it could be really horrible. > > > > > In general it's not easy to determine how paranoid we should be in non-debug > > code, but I'm not sure what's the need here specifically. > > I'm not sure how strict we should be but we definitely should try to > catch refcounting mistakes and that's my goal here. Yes I think it is worth it here (obviously :) > > >
