linux-kernel - Re: [PATCH] mm, debug: mm-introduce-vm_bug_on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 15 Oct 2014 11:03:59 +0200
From:	Michal Hocko <mhocko@...e.cz>
To:	Dave Jones <davej@...hat.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
	Sasha Levin <sasha.levin@...cle.com>
Subject: Re: [PATCH] mm, debug: mm-introduce-vm_bug_on_mm-fix-fix.patch

On Tue 14-10-14 12:50:27, Dave Jones wrote:
> On Tue, Oct 14, 2014 at 01:55:54PM +0200, Michal Hocko wrote:
> 
>  > > -#ifdef CONFIG_NUMA_BALANCING
>  > > -		"numa_next_scan %lu numa_scan_offset %lu numa_scan_seq %d\n"
>  > > -#endif
>  > > -#if defined(CONFIG_NUMA_BALANCING) || defined(CONFIG_COMPACTION)
>  > > -		"tlb_flush_pending %d\n"
>  > > -#endif
>  > > -		"%s",	/* This is here to hold the comma */
>  > > +	char *p = dumpmm_buffer;
>  > > +
>  > > +	memset(dumpmm_buffer, 0, 4096);
>  > 
>  > I do not see any locking here. Previously we had internal printk log as
>  > a natural synchronization. Now two threads are allowed to scribble over
>  > their messages leaving an unusable output in a better case.
> 
> That's why I asked in the part of the mail you didn't quote whether
> we cared if it wasn't reentrant. 

Ups, missed that. Sorry!

> Ok we do. That's 3 lines of change to add a lock.
> 
>  > Besides that the %s with "" trick is not really that ugly and handles
>  > the situation quite nicely. So do we really want to make it more
>  > complicated?
> 
> That hack goes away entirely with this diff.  And by keeping the
> parameters with the format string they're associated with, it should be
> more maintainable should we decide to add more fields to be output in the future.
> The number of ifdefs in the function are halved (which becomes even
> bigger deal if we do add more output).
> 
> We saw how many times we had to go around to get it right this time.
> In its current incarnation, it looks like a matter of time before
> someone screws it up again due to missing some CONFIG combination.

I do not have a strong opinion. I find the hack sufficient but it is
true that we have to be careful to not lose it in "a cleanup" or when
somebody adds new conditional fields behind. On the other hand it is
easier to manage potential overflow within printk rather than relying
on separate sprintk-s (the current code already looks like it can
consume close to 1k - but I haven't measured that).

Up to Andrew I guess.
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/