netdev - Re: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150108120203.GA7223@zenon.in.qult.net>
Date:	Thu, 8 Jan 2015 13:02:03 +0100
From:	'Ignacy Gawedzki' <ignacy.gawedzki@...en-communications.fr>
To:	David Laight <David.Laight@...LAB.COM>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later
 use

On Thu, Jan 08, 2015 at 11:05:24AM +0000, thus spake David Laight:
> This rather implies that you are calling kmalloc() with a spin lock help.
> If this is valid at all then you should probably specify GPF_ATOMIC.

Yes, you're right, my mistake.

> OTOH it is better to call kmalloc() before acquiring the lock.

This would certainly be the best solution, but still, it looks pretty hard to
implement this way since the whole sequence of functions is called from
tc_fill_qdisc() and tc_fill_tclass() in net/sched/sch_api.c: first a call to
gnet_stats_start_copy_compat() acquires the lock, then a call to per-qdisc
dump_stats() is performed that itself calls gnet_stats_copy_app() with the
pointer to the automatic structure that needs to be duplicated and finally
gnet_stats_finish_copy() is called that eventually releases the lock.  In the
originaly code, only gnet_stats_copy() can cause a failure and so the release
of the lock is performed there in such a case.

I don't see any easy way of knowing how much to allocate *before* the call to
gnet_stats_start_copy_compat().

> The locking itself looks odd - since the corresponding spin_lock_bh()
> isn't in the same function.

I agree that this doesn't look too good.

For the moment I'll post a corrected patch that uses GPF_ATOMIC.  Then anyone
can come up with a better fix anytime.

Ignacy

-- 
Ignacy Gawędzki
R&D Engineer
Green Communications
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html