Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031417AbbD2AfV (ORCPT ); Tue, 28 Apr 2015 20:35:21 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:44873 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031083AbbD2AfS (ORCPT ); Tue, 28 Apr 2015 20:35:18 -0400 Date: Wed, 29 Apr 2015 01:35:14 +0100 From: Al Viro To: Linus Torvalds Cc: Chris Metcalf , Fabian Frederick , Linux Kernel Mailing List , Randy Dunlap , Rickard Strandqvist Subject: Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy" Message-ID: <20150429003514.GL889@ZenIV.linux.org.uk> References: <20150428034859.GI889@ZenIV.linux.org.uk> <1363736428.511175.1430199310500.open-xchange@webmail.nmp.proximus.be> <20150428160523.GJ889@ZenIV.linux.org.uk> <553FE3F0.4060803@ezchip.com> <553FFDE9.9070700@ezchip.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2639 Lines: 51 On Tue, Apr 28, 2015 at 02:48:45PM -0700, Linus Torvalds wrote: > I suspect you could take that lib/strncpy_from_user.c and massage it > reasonably trivially to be a good function. > > That said, I can't think of a single strncpy (or strlcpy) in kernel > space that is actually worth even optimizing for. They just don't tend > to be in the critical path. So correctness is likely *much* more > important than worrying about performance. Indeed. As it is, I suspect that strlcpy() use should be uniformly discouraged; if nothing else, snprintf() gives the same semantics, is less likely to cause confusion regardling the expected return value and none of those paths are performance-critical. strncpy() has another use, though, and it can't be replaced by strlcpy() - see the commits that had started this thread. IMO they (and anything else of the same nature) really need to be reverted; using strlcpy() on something that isn't guaranteed to be NUL-terminated is a serious bug. And catching all such places is going to be quite a work - there are too many strlcpy() callers out there. Frankly, looking through call sites in fs... * two callers in fs/9p - strlcpy() + sscanf(), both should've been plain sscanf() (and the second should've been "HARDLINKCOUNT%u" instead of "%13s %u" + comparison of string with "HARDLINKCOUNT" - sscanf() is quite capable of matching explicit string literals) * affs one: match_strdup + strlcpy + kfree. Should just use match_strlcpy instead (BTW, despite the name, it does *not* use strclpy() internally). * afs: might be correct. * two in befs: both broken. * binfmt_misc: fishy; load_misc_binary() finds an object under rwlock, copies one of its fields (->interpreter), drops rwlock and proceeds to do various blocking operations (including open_exec()). Using another field of the same object (->interp_flags) all along. If it gets freed and reused, well... let's hope we won't fuck up too badly. IMO we'd be better off if we added a refcount to that sucker and used it to control the freeing. * btrfs: undefined behaviour - potentially overlapping source and destination. * another btrfs one: char b[BDEVNAME_SIZE]; strlcpy(s->s_id, bdevname(bdev, b), sizeof(s->s_id)); complete garbage; might as well do bdevname(bdev, s->s_id) and be done with that. ... and so on; this stuff is misused more often than not. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/