Paths

Table of Contentst

-
sys/net/
-
net/
-
if.c
1/2
if_var.h

Differential D28836

Add if_try_ref() to simplify refcount handling inside epoch
ClosedPublic
Actions

Authored by melifaro on Feb 21 2021, 2:00 PM.

Details

Reviewers

None

Group Reviewers

network

Commits

rG04e967d7270e: Add if_try_ref() to simplify refcount handling inside epoch.
rG7563019bc693: Add if_try_ref() to simplify refcount handling inside epoch.

Summary

More and more code migrates from lock-based protection to the NET_EPOCH umbrella.
It requires some logic changes, including, notably, refcount handling.

When we have an ifp pointer and we're running inside epoch we're guaranteed that this pointer will not be freed.
However, the following case can still happen:

in thread 1 we drop to 0 refcount for ifp and schedule its deletion.
in thread 2 we use this ifp and reference it
destroy callout kicks in
unhappy user reports bug

This can happen with the current implementation of ifnet_byindex_ref(), as we're not holding any locks preventing ifnet deletion by a parallel thread.

To address it, if_try_ref() function is added, allowing to return failure when we try to reference ifp with 0 refcount.
Additionally, existing if_ref() is enforced with KASSERT to provide a cleaner error in such scenarios.

Finally, fix ifnet_byindex_ref() by using if_try_ref() and returning NULL if the latter fails.

This is pretty much the same as D28639.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

melifaro created this revision.Feb 21 2021, 2:00 PM

Herald added subscribers: ae, imp. · View Herald TranscriptFeb 21 2021, 2:00 PM

Harbormaster completed remote builds in B37241: Diff 84398.Feb 21 2021, 2:00 PM

melifaro requested review of this revision.Feb 21 2021, 2:00 PM

melifaro retitled this revision from Add if_try_ref() failing when current refcount=0. Fix ifnet_byindex_ref() by using if_try_ref() instead of if_ref(). to Add if_try_ref() to simplify refcount handling inside epoch.Feb 21 2021, 2:08 PM

melifaro edited the summary of this revision. (Show Details)

melifaro added a reviewer: network.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 22 2021, 11:38 PM

Closed by commit rG7563019bc693: Add if_try_ref() to simplify refcount handling inside epoch. (authored by melifaro). · Explain Why

This revision was automatically updated to reflect the committed changes.

melifaro added a commit: rG7563019bc693: Add if_try_ref() to simplify refcount handling inside epoch..

melifaro added a commit: rG04e967d7270e: Add if_try_ref() to simplify refcount handling inside epoch..Sep 7 2021, 9:14 PM

Revision Contents
Changeset List

Path

Size

sys/

net/

if.c

14 lines

if_var.h

1 line

Diff 84499

View Options

sys/net/if.c

Show First 20 Lines • Show All 131 Lines • ▼ Show 20 Lines
#define is_kernel_pmap(x) ((x) == kernel_pmap)		#define is_kernel_pmap(x) ((x) == kernel_pmap)

struct pmap kernel_pmap_store;		struct pmap kernel_pmap_store;
pd_entry_t *kernel_segmap;		pd_entry_t *kernel_segmap;

vm_offset_t virtual_avail; /* VA of first avail page (after kernel bss) */		vm_offset_t virtual_avail; /* VA of first avail page (after kernel bss) */
vm_offset_t virtual_end; /* VA of last avail page (end of kernel AS) */		vm_offset_t virtual_end; /* VA of last avail page (end of kernel AS) */

		static int need_local_mappings;

static int nkpt;		static int nkpt;
unsigned pmap_max_asid; /* max ASID supported by the system */		unsigned pmap_max_asid; /* max ASID supported by the system */

#define PMAP_ASID_RESERVED 0		#define PMAP_ASID_RESERVED 0

vm_offset_t kernel_vm_end = VM_MIN_KERNEL_ADDRESS;		vm_offset_t kernel_vm_end = VM_MIN_KERNEL_ADDRESS;

static void pmap_asid_alloc(pmap_t pmap);		static void pmap_asid_alloc(pmap_t pmap);
Show All 33 Lines
static int pmap_unuse_pt(pmap_t, vm_offset_t, pd_entry_t);		static int pmap_unuse_pt(pmap_t, vm_offset_t, pd_entry_t);
static pt_entry_t init_pte_prot(vm_page_t m, vm_prot_t access, vm_prot_t prot);		static pt_entry_t init_pte_prot(vm_page_t m, vm_prot_t access, vm_prot_t prot);

static void pmap_invalidate_page_action(void *arg);		static void pmap_invalidate_page_action(void *arg);
static void pmap_invalidate_range_action(void *arg);		static void pmap_invalidate_range_action(void *arg);
static void pmap_update_page_action(void *arg);		static void pmap_update_page_action(void *arg);

#ifndef __mips_n64		#ifndef __mips_n64

		static vm_offset_t crashdumpva;

/*		/*
* This structure is for high memory (memory above 512Meg in 32 bit) support.		* These functions are for high memory (memory above 512Meg in 32 bit) support.
* The highmem area does not have a KSEG0 mapping, and we need a mechanism to		* The highmem area does not have a KSEG0 mapping, and we need a mechanism to
* do temporary per-CPU mappings for pmap_zero_page, pmap_copy_page etc.		* do temporary per-CPU mappings for pmap_zero_page, pmap_copy_page etc.
*		*
* At bootup, we reserve 2 virtual pages per CPU for mapping highmem pages. To		* At bootup, we reserve 2 virtual pages per CPU for mapping highmem pages. To
* access a highmem physical address on a CPU, we map the physical address to		* access a highmem physical address on a CPU, we map the physical address to
* the reserved virtual address for the CPU in the kernel pagetable. This is		* the reserved virtual address for the CPU in the kernel pagetable.
* done with interrupts disabled(although a spinlock and sched_pin would be
* sufficient).
*/		*/
struct local_sysmaps {
vm_offset_t base;
uint32_t saved_intr;
uint16_t valid1, valid2;
};
static struct local_sysmaps sysmap_lmem[MAXCPU];

static __inline void		static void
pmap_alloc_lmem_map(void)		pmap_init_reserved_pages(void)
		jahAuthorUnsubmitted Done Inline Actions If mips32 + SMP ends up not being a thing anymore due to the recent 64-bit atomic debate, this function can just be removed. Otherwise, is there a good way for me to exercise that config? I'm open to buying real HW... jah: If mips32 + SMP ends up not being a thing anymore due to the recent 64-bit atomic debate, this…
{		{
		struct pcpu *pc;
		vm_offset_t pages;
int i;		int i;

for (i = 0; i < MAXCPU; i++) {		if (need_local_mappings == 0)
sysmap_lmem[i].base = virtual_avail;		return;
virtual_avail += PAGE_SIZE * 2;
sysmap_lmem[i].valid1 = sysmap_lmem[i].valid2 = 0;		CPU_FOREACH(i) {
		pc = pcpu_find(i);
		/*
		* Skip if the mapping has already been initialized,
		* i.e. this is the BSP.
		*/
		if (pc->pc_cmap1_addr != 0)
		continue;
		pages = kva_alloc(PAGE_SIZE * 3);
		if (pages == 0)
		panic("%s: unable to allocate KVA", __func__);
		pc->pc_cmap1_ptep = pmap_pte(kernel_pmap, pages);
		pc->pc_cmap2_ptep = pmap_pte(kernel_pmap, pages + PAGE_SIZE);
		pc->pc_qmap_ptep =
		pmap_pte(kernel_pmap, pages + (PAGE_SIZE * 2));
		pc->pc_cmap1_addr = pages;
		pc->pc_cmap2_addr = pages + PAGE_SIZE;
		pc->pc_qmap_addr = pages + (PAGE_SIZE * 2);
}		}
}		}
		SYSINIT(rpages_init, SI_SUB_CPU, SI_ORDER_ANY, pmap_init_reserved_pages, NULL);

		static __inline void
		pmap_alloc_lmem_map(void)
		{
		PCPU_SET(cmap1_addr, virtual_avail);
		PCPU_SET(cmap2_addr, virtual_avail + PAGE_SIZE);
		PCPU_SET(cmap1_ptep, pmap_pte(kernel_pmap, virtual_avail));
		PCPU_SET(cmap2_ptep, pmap_pte(kernel_pmap, virtual_avail + PAGE_SIZE));
		PCPU_SET(qmap_addr, virtual_avail + (2 * PAGE_SIZE));
		PCPU_SET(qmap_ptep, pmap_pte(kernel_pmap, virtual_avail + (2 * PAGE_SIZE)));
		crashdumpva = virtual_avail + (3 * PAGE_SIZE);
		virtual_avail += PAGE_SIZE * 4;
		}

static __inline vm_offset_t		static __inline vm_offset_t
pmap_lmem_map1(vm_paddr_t phys)		pmap_lmem_map1(vm_paddr_t phys)
{		{
struct local_sysmaps *sysm;		critical_enter();
pt_entry_t *pte, npte;		*PCPU_GET(cmap1_ptep) =
vm_offset_t va;		TLBLO_PA_TO_PFN(phys) \| PTE_C_CACHE \| PTE_D \| PTE_V \| PTE_G;
uint32_t intr;		return (PCPU_GET(cmap1_addr));
int cpu;

intr = intr_disable();
cpu = PCPU_GET(cpuid);
sysm = &sysmap_lmem[cpu];
sysm->saved_intr = intr;
va = sysm->base;
npte = TLBLO_PA_TO_PFN(phys) \| PTE_C_CACHE \| PTE_D \| PTE_V \| PTE_G;
pte = pmap_pte(kernel_pmap, va);
*pte = npte;
sysm->valid1 = 1;
return (va);
}		}

static __inline vm_offset_t		static __inline vm_offset_t
pmap_lmem_map2(vm_paddr_t phys1, vm_paddr_t phys2)		pmap_lmem_map2(vm_paddr_t phys1, vm_paddr_t phys2)
{		{
struct local_sysmaps *sysm;		critical_enter();
pt_entry_t *pte, npte;		*PCPU_GET(cmap1_ptep) =
vm_offset_t va1, va2;		TLBLO_PA_TO_PFN(phys1) \| PTE_C_CACHE \| PTE_D \| PTE_V \| PTE_G;
uint32_t intr;		*PCPU_GET(cmap2_ptep) =
int cpu;		TLBLO_PA_TO_PFN(phys2) \| PTE_C_CACHE \| PTE_D \| PTE_V \| PTE_G;
		return (PCPU_GET(cmap1_addr));
intr = intr_disable();
cpu = PCPU_GET(cpuid);
sysm = &sysmap_lmem[cpu];
sysm->saved_intr = intr;
va1 = sysm->base;
va2 = sysm->base + PAGE_SIZE;
npte = TLBLO_PA_TO_PFN(phys1) \| PTE_C_CACHE \| PTE_D \| PTE_V \| PTE_G;
pte = pmap_pte(kernel_pmap, va1);
*pte = npte;
npte = TLBLO_PA_TO_PFN(phys2) \| PTE_C_CACHE \| PTE_D \| PTE_V \| PTE_G;
pte = pmap_pte(kernel_pmap, va2);
*pte = npte;
sysm->valid1 = 1;
sysm->valid2 = 1;
return (va1);
}		}

static __inline void		static __inline void
pmap_lmem_unmap(void)		pmap_lmem_unmap(void)
{		{
struct local_sysmaps *sysm;		*PCPU_GET(cmap1_ptep) = PTE_G;
pt_entry_t *pte;		tlb_invalidate_address(kernel_pmap, PCPU_GET(cmap1_addr));
int cpu;		if (*PCPU_GET(cmap2_ptep) != PTE_G) {
		*PCPU_GET(cmap2_ptep) = PTE_G;
cpu = PCPU_GET(cpuid);		tlb_invalidate_address(kernel_pmap, PCPU_GET(cmap2_addr));
sysm = &sysmap_lmem[cpu];
pte = pmap_pte(kernel_pmap, sysm->base);
*pte = PTE_G;
tlb_invalidate_address(kernel_pmap, sysm->base);
sysm->valid1 = 0;
if (sysm->valid2) {
pte = pmap_pte(kernel_pmap, sysm->base + PAGE_SIZE);
*pte = PTE_G;
tlb_invalidate_address(kernel_pmap, sysm->base + PAGE_SIZE);
sysm->valid2 = 0;
}		}
intr_restore(sysm->saved_intr);		critical_exit();
}		}

#else /* __mips_n64 */		#else /* __mips_n64 */

static __inline void		static __inline void
pmap_alloc_lmem_map(void)		pmap_alloc_lmem_map(void)
{		{
}		}

static __inline vm_offset_t		static __inline vm_offset_t
▲ Show 20 Lines • Show All 194 Lines • ▼ Show 20 Lines	#endif
kernel_pmap->pm_asid[0].gen = 0;		kernel_pmap->pm_asid[0].gen = 0;
kernel_vm_end += nkpt * NPTEPG * PAGE_SIZE;		kernel_vm_end += nkpt * NPTEPG * PAGE_SIZE;
}		}

void		void
pmap_bootstrap(void)		pmap_bootstrap(void)
{		{
int i;		int i;
int need_local_mappings = 0;

/* Sort. */		/* Sort. */
again:		again:
for (i = 0; phys_avail[i + 1] != 0; i += 2) {		for (i = 0; phys_avail[i + 1] != 0; i += 2) {
/*		/*
* Keep the memory aligned on page boundary.		* Keep the memory aligned on page boundary.
*/		*/
phys_avail[i] = round_page(phys_avail[i]);		phys_avail[i] = round_page(phys_avail[i]);
▲ Show 20 Lines • Show All 76 Lines • ▼ Show 20 Lines	#ifdef SMP
* in KSEG0 so there was no need for a TLB mapping.		* in KSEG0 so there was no need for a TLB mapping.
*/		*/
mips_pcpu_tlb_init(PCPU_ADDR(0));		mips_pcpu_tlb_init(PCPU_ADDR(0));

if (bootverbose)		if (bootverbose)
printf("pcpu is available at virtual address %p.\n", pcpup);		printf("pcpu is available at virtual address %p.\n", pcpup);
#endif		#endif

		pmap_create_kernel_pagetable();
if (need_local_mappings)		if (need_local_mappings)
pmap_alloc_lmem_map();		pmap_alloc_lmem_map();
pmap_create_kernel_pagetable();
pmap_max_asid = VMNUM_PIDS;		pmap_max_asid = VMNUM_PIDS;
mips_wr_entryhi(0);		mips_wr_entryhi(0);
mips_wr_pagemask(0);		mips_wr_pagemask(0);

/*		/*
* Initialize the global pv list lock.		* Initialize the global pv list lock.
*/		*/
rw_init(&pvh_global_lock, "pmap pv global");		rw_init(&pvh_global_lock, "pmap pv global");
▲ Show 20 Lines • Show All 1,740 Lines • ▼ Show 20 Lines	pmap_kenter_temporary(vm_paddr_t pa, int i)
if (i != 0)		if (i != 0)
printf("%s: ERROR!!! More than one page of virtual address mapping not supported\n",		printf("%s: ERROR!!! More than one page of virtual address mapping not supported\n",
__func__);		__func__);

if (MIPS_DIRECT_MAPPABLE(pa)) {		if (MIPS_DIRECT_MAPPABLE(pa)) {
va = MIPS_PHYS_TO_DIRECT(pa);		va = MIPS_PHYS_TO_DIRECT(pa);
} else {		} else {
#ifndef __mips_n64 /* XXX : to be converted to new style */		#ifndef __mips_n64 /* XXX : to be converted to new style */
int cpu;
register_t intr;
struct local_sysmaps *sysm;
pt_entry_t *pte, npte;		pt_entry_t *pte, npte;

/* If this is used other than for dumps, we may need to leave		pte = pmap_pte(kernel_pmap, crashdumpva);
* interrupts disasbled on return. If crash dumps don't work when
* we get to this point, we might want to consider this (leaving things
* disabled as a starting point ;-)
*/
intr = intr_disable();
cpu = PCPU_GET(cpuid);
sysm = &sysmap_lmem[cpu];
/* Since this is for the debugger, no locks or any other fun */		/* Since this is for the debugger, no locks or any other fun */
npte = TLBLO_PA_TO_PFN(pa) \| PTE_C_CACHE \| PTE_D \| PTE_V \|		npte = TLBLO_PA_TO_PFN(pa) \| PTE_C_CACHE \| PTE_D \| PTE_V \|
PTE_G;		PTE_G;
pte = pmap_pte(kernel_pmap, sysm->base);
*pte = npte;		*pte = npte;
sysm->valid1 = 1;		pmap_update_page(kernel_pmap, crashdumpva, npte);
pmap_update_page(kernel_pmap, sysm->base, npte);		va = crashdumpva;
va = sysm->base;
intr_restore(intr);
#endif		#endif
}		}
return ((void *)va);		return ((void *)va);
}		}

void		void
pmap_kenter_temporary_free(vm_paddr_t pa)		pmap_kenter_temporary_free(vm_paddr_t pa)
{		{
#ifndef __mips_n64 /* XXX : to be converted to new style */		#ifndef __mips_n64 /* XXX : to be converted to new style */
int cpu;		pt_entry_t *pte;
register_t intr;
struct local_sysmaps *sysm;
#endif		#endif

if (MIPS_DIRECT_MAPPABLE(pa)) {		if (MIPS_DIRECT_MAPPABLE(pa)) {
/* nothing to do for this case */		/* nothing to do for this case */
return;		return;
}		}
#ifndef __mips_n64 /* XXX : to be converted to new style */		#ifndef __mips_n64 /* XXX : to be converted to new style */
cpu = PCPU_GET(cpuid);		pte = pmap_pte(kernel_pmap, crashdumpva);
sysm = &sysmap_lmem[cpu];
if (sysm->valid1) {
pt_entry_t *pte;

intr = intr_disable();
pte = pmap_pte(kernel_pmap, sysm->base);
*pte = PTE_G;		*pte = PTE_G;
pmap_invalidate_page(kernel_pmap, sysm->base);		pmap_invalidate_page(kernel_pmap, crashdumpva);
intr_restore(intr);
sysm->valid1 = 0;
}
#endif		#endif
}		}

/*		/*
* Maps a sequence of resident pages belonging to the same object.		* Maps a sequence of resident pages belonging to the same object.
* The sequence begins with the given page m_start. This page is		* The sequence begins with the given page m_start. This page is
* mapped at the given virtual address start. Each subsequent page is		* mapped at the given virtual address start. Each subsequent page is
* mapped at a virtual address that is offset from start by the same		* mapped at a virtual address that is offset from start by the same
▲ Show 20 Lines • Show All 237 Lines • ▼ Show 20 Lines
}		}

vm_offset_t		vm_offset_t
pmap_quick_enter_page(vm_page_t m)		pmap_quick_enter_page(vm_page_t m)
{		{
#if defined(__mips_n64)		#if defined(__mips_n64)
return MIPS_PHYS_TO_DIRECT(VM_PAGE_TO_PHYS(m));		return MIPS_PHYS_TO_DIRECT(VM_PAGE_TO_PHYS(m));
#else		#else
		vm_offset_t qaddr;
vm_paddr_t pa;		vm_paddr_t pa;
struct local_sysmaps *sysm;
pt_entry_t *pte, npte;		pt_entry_t *pte, npte;

pa = VM_PAGE_TO_PHYS(m);		pa = VM_PAGE_TO_PHYS(m);

if (MIPS_DIRECT_MAPPABLE(pa)) {		if (MIPS_DIRECT_MAPPABLE(pa)) {
if (pmap_page_get_memattr(m) != VM_MEMATTR_WRITE_BACK)		if (pmap_page_get_memattr(m) != VM_MEMATTR_WRITE_BACK)
return (MIPS_PHYS_TO_DIRECT_UNCACHED(pa));		return (MIPS_PHYS_TO_DIRECT_UNCACHED(pa));
else		else
return (MIPS_PHYS_TO_DIRECT(pa));		return (MIPS_PHYS_TO_DIRECT(pa));
}		}
critical_enter();		critical_enter();
sysm = &sysmap_lmem[PCPU_GET(cpuid)];		qaddr = PCPU_GET(qmap_addr);
		pte = PCPU_GET(qmap_ptep);

KASSERT(sysm->valid1 == 0, ("pmap_quick_enter_page: PTE busy"));		KASSERT(*pte == PTE_G, ("pmap_quick_enter_page: PTE busy"));

pte = pmap_pte(kernel_pmap, sysm->base);
npte = TLBLO_PA_TO_PFN(pa) \| PTE_D \| PTE_V \| PTE_G;		npte = TLBLO_PA_TO_PFN(pa) \| PTE_D \| PTE_V \| PTE_G;
PMAP_PTE_SET_CACHE_BITS(npte, pa, m);		PMAP_PTE_SET_CACHE_BITS(npte, pa, m);
*pte = npte;		*pte = npte;
sysm->valid1 = 1;

return (sysm->base);		return (qaddr);
#endif		#endif
}		}

void		void
pmap_quick_remove_page(vm_offset_t addr)		pmap_quick_remove_page(vm_offset_t addr)
{		{
mips_dcache_wbinv_range(addr, PAGE_SIZE);		mips_dcache_wbinv_range(addr, PAGE_SIZE);
		jahAuthorUnsubmitted Not Done Inline Actions One thing I've been curious about looking thru the mips pmap is the need to clean the cache every time we remove a mapping. It seems like MIPS d-caches are always VIPT these days, so I'd naively expect the physical tag to allow eviction to work even in the absence of a valid mapping. Is this just an architectural requirement I'm not familiar with, or is it a mitigation against cache aliasing, or something else entirely? I ask about the aliasing thing because I vaguely recall a post on -developers or -arch maybe ~3yrs ago that mentioned the pmap isn't doing what it should to prevent alias-related corruption. Based on the MIPS docs I've read, it does seem like that's possible with certain combinations of (large way size + small page size + no hardware anti-alias). jah: One thing I've been curious about looking thru the mips pmap is the need to clean the cache…

#if !defined(__mips_n64)		#if !defined(__mips_n64)
struct local_sysmaps *sysm;
pt_entry_t *pte;		pt_entry_t *pte;

if (addr >= MIPS_KSEG0_START && addr < MIPS_KSEG0_END)		if (addr >= MIPS_KSEG0_START && addr < MIPS_KSEG0_END)
return;		return;

sysm = &sysmap_lmem[PCPU_GET(cpuid)];		pte = PCPU_GET(qmap_ptep);

KASSERT(sysm->valid1 != 0,		KASSERT(*pte != PTE_G,
("pmap_quick_remove_page: PTE not in use"));		("pmap_quick_remove_page: PTE not in use"));
KASSERT(sysm->base == addr,		KASSERT(PCPU_GET(qmap_addr) == addr,
("pmap_quick_remove_page: invalid address"));		("pmap_quick_remove_page: invalid address"));

pte = pmap_pte(kernel_pmap, addr);
*pte = PTE_G;		*pte = PTE_G;
tlb_invalidate_address(kernel_pmap, addr);		tlb_invalidate_address(kernel_pmap, addr);
sysm->valid1 = 0;
critical_exit();		critical_exit();
#endif		#endif
}		}

/*		/*
* Returns true if the pmap's pv is one of the first		* Returns true if the pmap's pv is one of the first
* 16 pvs linked to from this page. This count may		* 16 pvs linked to from this page. This count may
* be changed upwards or downwards in the future; it		* be changed upwards or downwards in the future; it
▲ Show 20 Lines • Show All 951 Lines • Show Last 20 Lines