Hi Sasha, but why do you think it makes sense to backport these "uprobes" cleanups? Oleg. On 11/24, Sasha Levin wrote: > > From: Oleg Nesterov <oleg@xxxxxxxxxx> > > [ Upstream commit c7b4133c48445dde789ed30b19ccb0448c7593f7 ] > > 1. Clear utask->xol_vaddr unconditionally, even if this addr is not valid, > xol_free_insn_slot() should never return with utask->xol_vaddr != NULL. > > 2. Add a comment to explain why do we need to validate slot_addr. > > 3. Simplify the validation above. We can simply check offset < PAGE_SIZE, > unsigned underflows are fine, it should work if slot_addr < area->vaddr. > > 4. Kill the unnecessary "slot_nr >= UINSNS_PER_PAGE" check, slot_nr must > be valid if offset < PAGE_SIZE. > > The next patches will cleanup this function even more. > > Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx> > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx> > Link: https://lore.kernel.org/r/20240929144235.GA9471@xxxxxxxxxx > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx> > --- > kernel/events/uprobes.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 4b52cb2ae6d62..cc605df73d72f 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -1683,8 +1683,8 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe) > static void xol_free_insn_slot(struct task_struct *tsk) > { > struct xol_area *area; > - unsigned long vma_end; > unsigned long slot_addr; > + unsigned long offset; > > if (!tsk->mm || !tsk->mm->uprobes_state.xol_area || !tsk->utask) > return; > @@ -1693,24 +1693,21 @@ static void xol_free_insn_slot(struct task_struct *tsk) > if (unlikely(!slot_addr)) > return; > > + tsk->utask->xol_vaddr = 0; > area = tsk->mm->uprobes_state.xol_area; > - vma_end = area->vaddr + PAGE_SIZE; > - if (area->vaddr <= slot_addr && slot_addr < vma_end) { > - unsigned long offset; > - int slot_nr; > - > - offset = slot_addr - area->vaddr; > - slot_nr = offset / UPROBE_XOL_SLOT_BYTES; > - if (slot_nr >= UINSNS_PER_PAGE) > - return; > + offset = slot_addr - area->vaddr; > + /* > + * slot_addr must fit into [area->vaddr, area->vaddr + PAGE_SIZE). > + * This check can only fail if the "[uprobes]" vma was mremap'ed. > + */ > + if (offset < PAGE_SIZE) { > + int slot_nr = offset / UPROBE_XOL_SLOT_BYTES; > > clear_bit(slot_nr, area->bitmap); > atomic_dec(&area->slot_count); > smp_mb__after_atomic(); /* pairs with prepare_to_wait() */ > if (waitqueue_active(&area->wq)) > wake_up(&area->wq); > - > - tsk->utask->xol_vaddr = 0; > } > } > > -- > 2.43.0 >
