Revision 07b90056cb15ff9877dca0d8f1b6583d1051f724 authored by Vladimir Oltean on 11 January 2021, 23:09:43 UTC, committed by Jakub Kicinski on 13 January 2021, 02:48:40 UTC
Currently the following happens when a DSA master driver unbinds while
there are DSA switches attached to it:

$ echo 0000:00:00.5 > /sys/bus/pci/drivers/mscc_felix/unbind
------------[ cut here ]------------
WARNING: CPU: 0 PID: 392 at net/core/dev.c:9507
Call trace:
 rollback_registered_many+0x5fc/0x688
 unregister_netdevice_queue+0x98/0x120
 dsa_slave_destroy+0x4c/0x88
 dsa_port_teardown.part.16+0x78/0xb0
 dsa_tree_teardown_switches+0x58/0xc0
 dsa_unregister_switch+0x104/0x1b8
 felix_pci_remove+0x24/0x48
 pci_device_remove+0x48/0xf0
 device_release_driver_internal+0x118/0x1e8
 device_driver_detach+0x28/0x38
 unbind_store+0xd0/0x100

Located at the above location is this WARN_ON:

	/* Notifier chain MUST detach us all upper devices. */
	WARN_ON(netdev_has_any_upper_dev(dev));

Other stacked interfaces, like VLAN, do indeed listen for
NETDEV_UNREGISTER on the real_dev and also unregister themselves at that
time, which is clearly the behavior that rollback_registered_many
expects. But DSA interfaces are not VLAN. They have backing hardware
(platform devices, PCI devices, MDIO, SPI etc) which have a life cycle
of their own and we can't just trigger an unregister from the DSA
framework when we receive a netdev notifier that the master unregisters.

Luckily, there is something we can do, and that is to inform the driver
core that we have a runtime dependency to the DSA master interface's
device, and create a device link where that is the supplier and we are
the consumer. Having this device link will make the DSA switch unbind
before the DSA master unbinds, which is enough to avoid the WARN_ON from
rollback_registered_many.

Note that even before the blamed commit, DSA did nothing intelligent
when the master interface got unregistered either. See the discussion
here:
https://lore.kernel.org/netdev/20200505210253.20311-1-f.fainelli@gmail.com/
But this time, at least the WARN_ON is loud enough that the
upper_dev_link commit can be blamed.

The advantage with this approach vs dev_hold(master) in the attached
link is that the latter is not meant for long term reference counting.
With dev_hold, the only thing that will happen is that when the user
attempts an unbind of the DSA master, netdev_wait_allrefs will keep
waiting and waiting, due to DSA keeping the refcount forever. DSA would
not access freed memory corresponding to the master interface, but the
unbind would still result in a freeze. Whereas with device links,
graceful teardown is ensured. It even works with cascaded DSA trees.

$ echo 0000:00:00.2 > /sys/bus/pci/drivers/fsl_enetc/unbind
[ 1818.797546] device swp0 left promiscuous mode
[ 1819.301112] sja1105 spi2.0: Link is Down
[ 1819.307981] DSA: tree 1 torn down
[ 1819.312408] device eno2 left promiscuous mode
[ 1819.656803] mscc_felix 0000:00:00.5: Link is Down
[ 1819.667194] DSA: tree 0 torn down
[ 1819.711557] fsl_enetc 0000:00:00.2 eno2: Link is Down

This approach allows us to keep the DSA framework absolutely unchanged,
and the driver core will just know to unbind us first when the master
goes away - as opposed to the large (and probably impossible) rework
required if attempting to listen for NETDEV_UNREGISTER.

As per the documentation at Documentation/driver-api/device_link.rst,
specifying the DL_FLAG_AUTOREMOVE_CONSUMER flag causes the device link
to be automatically purged when the consumer fails to probe or later
unbinds. So we don't need to keep the consumer_link variable in struct
dsa_switch.

Fixes: 2f1e8ea726e9 ("net: dsa: link interfaces with the DSA master to get rid of lockdep warnings")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
Link: https://lore.kernel.org/r/20210111230943.3701806-1-olteanv@gmail.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
1 parent a18caa9
Raw File
mapping_dirty_helpers.c
// SPDX-License-Identifier: GPL-2.0
#include <linux/pagewalk.h>
#include <linux/hugetlb.h>
#include <linux/bitops.h>
#include <linux/mmu_notifier.h>
#include <asm/cacheflush.h>
#include <asm/tlbflush.h>

/**
 * struct wp_walk - Private struct for pagetable walk callbacks
 * @range: Range for mmu notifiers
 * @tlbflush_start: Address of first modified pte
 * @tlbflush_end: Address of last modified pte + 1
 * @total: Total number of modified ptes
 */
struct wp_walk {
	struct mmu_notifier_range range;
	unsigned long tlbflush_start;
	unsigned long tlbflush_end;
	unsigned long total;
};

/**
 * wp_pte - Write-protect a pte
 * @pte: Pointer to the pte
 * @addr: The start of protecting virtual address
 * @end: The end of protecting virtual address
 * @walk: pagetable walk callback argument
 *
 * The function write-protects a pte and records the range in
 * virtual address space of touched ptes for efficient range TLB flushes.
 */
static int wp_pte(pte_t *pte, unsigned long addr, unsigned long end,
		  struct mm_walk *walk)
{
	struct wp_walk *wpwalk = walk->private;
	pte_t ptent = *pte;

	if (pte_write(ptent)) {
		pte_t old_pte = ptep_modify_prot_start(walk->vma, addr, pte);

		ptent = pte_wrprotect(old_pte);
		ptep_modify_prot_commit(walk->vma, addr, pte, old_pte, ptent);
		wpwalk->total++;
		wpwalk->tlbflush_start = min(wpwalk->tlbflush_start, addr);
		wpwalk->tlbflush_end = max(wpwalk->tlbflush_end,
					   addr + PAGE_SIZE);
	}

	return 0;
}

/**
 * struct clean_walk - Private struct for the clean_record_pte function.
 * @base: struct wp_walk we derive from
 * @bitmap_pgoff: Address_space Page offset of the first bit in @bitmap
 * @bitmap: Bitmap with one bit for each page offset in the address_space range
 * covered.
 * @start: Address_space page offset of first modified pte relative
 * to @bitmap_pgoff
 * @end: Address_space page offset of last modified pte relative
 * to @bitmap_pgoff
 */
struct clean_walk {
	struct wp_walk base;
	pgoff_t bitmap_pgoff;
	unsigned long *bitmap;
	pgoff_t start;
	pgoff_t end;
};

#define to_clean_walk(_wpwalk) container_of(_wpwalk, struct clean_walk, base)

/**
 * clean_record_pte - Clean a pte and record its address space offset in a
 * bitmap
 * @pte: Pointer to the pte
 * @addr: The start of virtual address to be clean
 * @end: The end of virtual address to be clean
 * @walk: pagetable walk callback argument
 *
 * The function cleans a pte and records the range in
 * virtual address space of touched ptes for efficient TLB flushes.
 * It also records dirty ptes in a bitmap representing page offsets
 * in the address_space, as well as the first and last of the bits
 * touched.
 */
static int clean_record_pte(pte_t *pte, unsigned long addr,
			    unsigned long end, struct mm_walk *walk)
{
	struct wp_walk *wpwalk = walk->private;
	struct clean_walk *cwalk = to_clean_walk(wpwalk);
	pte_t ptent = *pte;

	if (pte_dirty(ptent)) {
		pgoff_t pgoff = ((addr - walk->vma->vm_start) >> PAGE_SHIFT) +
			walk->vma->vm_pgoff - cwalk->bitmap_pgoff;
		pte_t old_pte = ptep_modify_prot_start(walk->vma, addr, pte);

		ptent = pte_mkclean(old_pte);
		ptep_modify_prot_commit(walk->vma, addr, pte, old_pte, ptent);

		wpwalk->total++;
		wpwalk->tlbflush_start = min(wpwalk->tlbflush_start, addr);
		wpwalk->tlbflush_end = max(wpwalk->tlbflush_end,
					   addr + PAGE_SIZE);

		__set_bit(pgoff, cwalk->bitmap);
		cwalk->start = min(cwalk->start, pgoff);
		cwalk->end = max(cwalk->end, pgoff + 1);
	}

	return 0;
}

/*
 * wp_clean_pmd_entry - The pagewalk pmd callback.
 *
 * Dirty-tracking should take place on the PTE level, so
 * WARN() if encountering a dirty huge pmd.
 * Furthermore, never split huge pmds, since that currently
 * causes dirty info loss. The pagefault handler should do
 * that if needed.
 */
static int wp_clean_pmd_entry(pmd_t *pmd, unsigned long addr, unsigned long end,
			      struct mm_walk *walk)
{
	pmd_t pmdval = pmd_read_atomic(pmd);

	if (!pmd_trans_unstable(&pmdval))
		return 0;

	if (pmd_none(pmdval)) {
		walk->action = ACTION_AGAIN;
		return 0;
	}

	/* Huge pmd, present or migrated */
	walk->action = ACTION_CONTINUE;
	if (pmd_trans_huge(pmdval) || pmd_devmap(pmdval))
		WARN_ON(pmd_write(pmdval) || pmd_dirty(pmdval));

	return 0;
}

/*
 * wp_clean_pud_entry - The pagewalk pud callback.
 *
 * Dirty-tracking should take place on the PTE level, so
 * WARN() if encountering a dirty huge puds.
 * Furthermore, never split huge puds, since that currently
 * causes dirty info loss. The pagefault handler should do
 * that if needed.
 */
static int wp_clean_pud_entry(pud_t *pud, unsigned long addr, unsigned long end,
			      struct mm_walk *walk)
{
	pud_t pudval = READ_ONCE(*pud);

	if (!pud_trans_unstable(&pudval))
		return 0;

	if (pud_none(pudval)) {
		walk->action = ACTION_AGAIN;
		return 0;
	}

	/* Huge pud */
	walk->action = ACTION_CONTINUE;
	if (pud_trans_huge(pudval) || pud_devmap(pudval))
		WARN_ON(pud_write(pudval) || pud_dirty(pudval));

	return 0;
}

/*
 * wp_clean_pre_vma - The pagewalk pre_vma callback.
 *
 * The pre_vma callback performs the cache flush, stages the tlb flush
 * and calls the necessary mmu notifiers.
 */
static int wp_clean_pre_vma(unsigned long start, unsigned long end,
			    struct mm_walk *walk)
{
	struct wp_walk *wpwalk = walk->private;

	wpwalk->tlbflush_start = end;
	wpwalk->tlbflush_end = start;

	mmu_notifier_range_init(&wpwalk->range, MMU_NOTIFY_PROTECTION_PAGE, 0,
				walk->vma, walk->mm, start, end);
	mmu_notifier_invalidate_range_start(&wpwalk->range);
	flush_cache_range(walk->vma, start, end);

	/*
	 * We're not using tlb_gather_mmu() since typically
	 * only a small subrange of PTEs are affected, whereas
	 * tlb_gather_mmu() records the full range.
	 */
	inc_tlb_flush_pending(walk->mm);

	return 0;
}

/*
 * wp_clean_post_vma - The pagewalk post_vma callback.
 *
 * The post_vma callback performs the tlb flush and calls necessary mmu
 * notifiers.
 */
static void wp_clean_post_vma(struct mm_walk *walk)
{
	struct wp_walk *wpwalk = walk->private;

	if (mm_tlb_flush_nested(walk->mm))
		flush_tlb_range(walk->vma, wpwalk->range.start,
				wpwalk->range.end);
	else if (wpwalk->tlbflush_end > wpwalk->tlbflush_start)
		flush_tlb_range(walk->vma, wpwalk->tlbflush_start,
				wpwalk->tlbflush_end);

	mmu_notifier_invalidate_range_end(&wpwalk->range);
	dec_tlb_flush_pending(walk->mm);
}

/*
 * wp_clean_test_walk - The pagewalk test_walk callback.
 *
 * Won't perform dirty-tracking on COW, read-only or HUGETLB vmas.
 */
static int wp_clean_test_walk(unsigned long start, unsigned long end,
			      struct mm_walk *walk)
{
	unsigned long vm_flags = READ_ONCE(walk->vma->vm_flags);

	/* Skip non-applicable VMAs */
	if ((vm_flags & (VM_SHARED | VM_MAYWRITE | VM_HUGETLB)) !=
	    (VM_SHARED | VM_MAYWRITE))
		return 1;

	return 0;
}

static const struct mm_walk_ops clean_walk_ops = {
	.pte_entry = clean_record_pte,
	.pmd_entry = wp_clean_pmd_entry,
	.pud_entry = wp_clean_pud_entry,
	.test_walk = wp_clean_test_walk,
	.pre_vma = wp_clean_pre_vma,
	.post_vma = wp_clean_post_vma
};

static const struct mm_walk_ops wp_walk_ops = {
	.pte_entry = wp_pte,
	.pmd_entry = wp_clean_pmd_entry,
	.pud_entry = wp_clean_pud_entry,
	.test_walk = wp_clean_test_walk,
	.pre_vma = wp_clean_pre_vma,
	.post_vma = wp_clean_post_vma
};

/**
 * wp_shared_mapping_range - Write-protect all ptes in an address space range
 * @mapping: The address_space we want to write protect
 * @first_index: The first page offset in the range
 * @nr: Number of incremental page offsets to cover
 *
 * Note: This function currently skips transhuge page-table entries, since
 * it's intended for dirty-tracking on the PTE level. It will warn on
 * encountering transhuge write-enabled entries, though, and can easily be
 * extended to handle them as well.
 *
 * Return: The number of ptes actually write-protected. Note that
 * already write-protected ptes are not counted.
 */
unsigned long wp_shared_mapping_range(struct address_space *mapping,
				      pgoff_t first_index, pgoff_t nr)
{
	struct wp_walk wpwalk = { .total = 0 };

	i_mmap_lock_read(mapping);
	WARN_ON(walk_page_mapping(mapping, first_index, nr, &wp_walk_ops,
				  &wpwalk));
	i_mmap_unlock_read(mapping);

	return wpwalk.total;
}
EXPORT_SYMBOL_GPL(wp_shared_mapping_range);

/**
 * clean_record_shared_mapping_range - Clean and record all ptes in an
 * address space range
 * @mapping: The address_space we want to clean
 * @first_index: The first page offset in the range
 * @nr: Number of incremental page offsets to cover
 * @bitmap_pgoff: The page offset of the first bit in @bitmap
 * @bitmap: Pointer to a bitmap of at least @nr bits. The bitmap needs to
 * cover the whole range @first_index..@first_index + @nr.
 * @start: Pointer to number of the first set bit in @bitmap.
 * is modified as new bits are set by the function.
 * @end: Pointer to the number of the last set bit in @bitmap.
 * none set. The value is modified as new bits are set by the function.
 *
 * Note: When this function returns there is no guarantee that a CPU has
 * not already dirtied new ptes. However it will not clean any ptes not
 * reported in the bitmap. The guarantees are as follows:
 * a) All ptes dirty when the function starts executing will end up recorded
 *    in the bitmap.
 * b) All ptes dirtied after that will either remain dirty, be recorded in the
 *    bitmap or both.
 *
 * If a caller needs to make sure all dirty ptes are picked up and none
 * additional are added, it first needs to write-protect the address-space
 * range and make sure new writers are blocked in page_mkwrite() or
 * pfn_mkwrite(). And then after a TLB flush following the write-protection
 * pick up all dirty bits.
 *
 * Note: This function currently skips transhuge page-table entries, since
 * it's intended for dirty-tracking on the PTE level. It will warn on
 * encountering transhuge dirty entries, though, and can easily be extended
 * to handle them as well.
 *
 * Return: The number of dirty ptes actually cleaned.
 */
unsigned long clean_record_shared_mapping_range(struct address_space *mapping,
						pgoff_t first_index, pgoff_t nr,
						pgoff_t bitmap_pgoff,
						unsigned long *bitmap,
						pgoff_t *start,
						pgoff_t *end)
{
	bool none_set = (*start >= *end);
	struct clean_walk cwalk = {
		.base = { .total = 0 },
		.bitmap_pgoff = bitmap_pgoff,
		.bitmap = bitmap,
		.start = none_set ? nr : *start,
		.end = none_set ? 0 : *end,
	};

	i_mmap_lock_read(mapping);
	WARN_ON(walk_page_mapping(mapping, first_index, nr, &clean_walk_ops,
				  &cwalk.base));
	i_mmap_unlock_read(mapping);

	*start = cwalk.start;
	*end = cwalk.end;

	return cwalk.base.total;
}
EXPORT_SYMBOL_GPL(clean_record_shared_mapping_range);
back to top