Re: [PATCH v3 3/3] mm,hwpoison: add kill_accessing_process() to find error virtual address

From: Borislav Petkov <bp@alien8.de>
To: Naoya Horiguchi <nao.horiguchi@gmail.com>
Cc: linux-mm@kvack.org, Tony Luck <tony.luck@intel.com>,
	Aili Yao <yaoaili@kingsoft.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Oscar Salvador <osalvador@suse.de>,
	David Hildenbrand <david@redhat.com>,
	Andy Lutomirski <luto@kernel.org>,
	Naoya Horiguchi <naoya.horiguchi@nec.com>,
	Jue Wang <juew@google.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/3] mm,hwpoison: add kill_accessing_process() to find error virtual address
Date: Thu, 22 Apr 2021 19:02:13 +0200	[thread overview]
Message-ID: <20210422170213.GE7021@zn.tnic> (raw)
In-Reply-To: <20210421005728.1994268-4-nao.horiguchi@gmail.com>

On Wed, Apr 21, 2021 at 09:57:28AM +0900, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> The previous patch solves the infinite MCE loop issue when multiple

"previous patch" has no meaning when it is in git.

> MCE events races.  The remaining issue is to make sure that all threads

	    "race."

> processing Action Required MCEs send to the current processes the

s/the //

> SIGBUS with the proper virtual address and the error size.
> 
> This patch suggests to do page table walk to find the error virtual

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> address.  If we find multiple virtual addresses in walking, we now can't

Who's "we"?				during the pagetable walk

> determine which one is correct, so we fall back to sending SIGBUS in
> kill_me_maybe() without error info as we do now.  This corner case needs
> to be solved in the future.

Solved how? If you can't map which error comes from which process, you
can't do anything here. You could send SIGBUS to all but you might
injure some innocent bystanders this way.

Just code structuring suggestions below - mm stuff is for someone else
to review properly.

> +static int hwpoison_pte_range(pmd_t *pmdp, unsigned long addr,
> +			      unsigned long end, struct mm_walk *walk)
> +{
> +	struct hwp_walk *hwp = (struct hwp_walk *)walk->private;
> +	int ret = 0;
> +	pte_t *ptep;
> +	spinlock_t *ptl;
> +
> +	ptl = pmd_trans_huge_lock(pmdp, walk->vma);
> +	if (ptl) {

Save yourself an indentation level:

	if (!ptl)
		goto unlock;

> +		pmd_t pmd = *pmdp;
> +
> +		if (pmd_present(pmd)) {

... ditto...

> +			unsigned long pfn = pmd_pfn(pmd);
> +
> +			if (pfn <= hwp->pfn && hwp->pfn < pfn + HPAGE_PMD_NR) {
> +				unsigned long hwpoison_vaddr = addr +
> +					((hwp->pfn - pfn) << PAGE_SHIFT);

... which will allow you to not break those.

> +
> +				ret = set_to_kill(&hwp->tk, hwpoison_vaddr,
> +						  PAGE_SHIFT);
> +			}
> +		}
> +		spin_unlock(ptl);
> +		goto out;
> +	}
> +
> +	if (pmd_trans_unstable(pmdp))
> +		goto out;
> +
> +	ptep = pte_offset_map_lock(walk->vma->vm_mm, pmdp, addr, &ptl);
> +	for (; addr != end; ptep++, addr += PAGE_SIZE) {
> +		ret = check_hwpoisoned_entry(*ptep, addr, PAGE_SHIFT,
> +					     hwp->pfn, &hwp->tk);
> +		if (ret == 1)
> +			break;
> +	}
> +	pte_unmap_unlock(ptep - 1, ptl);
> +out:
> +	cond_resched();
> +	return ret;
> +}

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette