Re: [PATCH v15 1/2] Reorganize the oom report in dump_header

From: 禹舟键 <ufo19890607@gmail.com>
To: mhocko@kernel.org
Cc: akpm@linux-foundation.org, rientjes@google.com,
	kirill.shutemov@linux.intel.com, aarcange@redhat.com,
	penguin-kernel@i-love.sakura.ne.jp, guro@fb.com,
	yang.s@alibaba-inc.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org,
	Wind Yu <yuzhoujian@didichuxing.com>
Subject: Re: [PATCH v15 1/2] Reorganize the oom report in dump_header
Date: Thu, 1 Nov 2018 18:09:39 +0800	[thread overview]
Message-ID: <CAHCio2jpqfdgrqOqyXQ=HUc-9kzDmtaYXH+9juVQS6hBHhSdPA@mail.gmail.com> (raw)
In-Reply-To: <20181031135049.GO32673@dhcp22.suse.cz>

[-- Attachment #1: Type: text/plain, Size: 2081 bytes --]

Hi Michal
The null pointer is possible when calling the dump_header, this bug was
detected by LKP. Below is the context 3 months ago.

On Mon 30-07-18 19:05:50, David Rientjes wrote:
> On Mon, 30 Jul 2018, Michal Hocko wrote:
>
> > On Mon 30-07-18 17:03:20, kernel test robot wrote:
> > [...]
> > > [    9.034310] BUG: KASAN: null-ptr-deref in dump_header+0x10c/0x448
> >
> > Could you faddr2line on the offset please?
> >
>
> It's possible that p is NULL when calling dump_header().  In this case we
> do not want to print any line concerning a victim because no oom kill has
> occurred.

> You are right. I have missed those.

> This code shouldn't be part of dump_header(), which is called from
> multiple contexts even when an oom kill has not occurred, and is
> ratelimited.  The single line output should be the canonical way that
> userspace parses the log for oom victims, we can't ratelimit it.
>
> The following would be a fix patch, but it will be broken if the cgroup
> aware oom killer is removed from -mm so that the oom_group stuff can be
> merged.

> cgroup aware oom killer is going to be replaced by a new implementation
> IIUC so the fix should be based on the yuzhoujian patch. Ideally to be
> resubmitted.

> I would just suggest adding it into a function
> dump_oom_summary(struct oom_control *oc, struct task_struct *p)

> yuzhoujian could you take care of that please?

I followed David's tip and call the new func dump_oom_summary in the
oom_kill_process.

> It's possible that p is NULL when calling dump_header().  In this case we
> do not want to print any line concerning a victim because no oom kill has
>occurred.

> This code shouldn't be part of dump_header(), which is called from
> multiple contexts even when an oom kill has not occurred, and is
> ratelimited.  The single line output should be the canonical way that
> userspace parses the log for oom victims, we can't ratelimit it.

> The following would be a fix patch, but it will be broken if the cgroup
> aware oom killer is removed from -mm so that the oom_group stuff can be
> merged.

[-- Attachment #2: Type: text/html, Size: 2673 bytes --]