Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756908AbYLKQGz (ORCPT ); Thu, 11 Dec 2008 11:06:55 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1756331AbYLKQGo (ORCPT ); Thu, 11 Dec 2008 11:06:44 -0500 Received: from ey-out-2122.google.com ([74.125.78.26]:41136 "EHLO ey-out-2122.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756248AbYLKQGm (ORCPT ); Thu, 11 Dec 2008 11:06:42 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references:x-google-sender-auth; b=aD5yZtXOP5nwrr9wYzq1mq2h5jE9PsWvpSnQEozwgJSa7oF7XgRLjrXXRmu3FBEglp iX0U26YvDHtlI9LiOePrdqNBj+ZLbIAoXJbcAn9o1afUOzIMseHGqZY1pFKJwEBg4jKw 1BT/pVSv7lPn8E7my7N4iCzWTPTP+08F4PUOU= Message-ID: Date: Thu, 11 Dec 2008 16:06:40 +0000 From: "Duane Griffin" To: "Boaz Harrosh" Subject: Re: Checking link targets are NULL-terminated Cc: "Christoph Hellwig" , "Andrew Morton" , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org In-Reply-To: <493EB22C.2050108@panasas.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20081205144810.GA25585@dastardly.home.dghda.com> <20081208143003.9449bc77.akpm@linux-foundation.org> <20081209171814.GA8667@infradead.org> <493EB22C.2050108@panasas.com> X-Google-Sender-Auth: f0cf9aa3c04c00bb Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4476 Lines: 103 2008/12/9 Boaz Harrosh : > Christoph Hellwig wrote: >> On Mon, Dec 08, 2008 at 02:30:03PM -0800, Andrew Morton wrote: >>> Perhaps nd_set_link() is a suitable place? Change that function so >>> that it is passed a third argument (max_len) and then check that within >>> nd_set_link(). Change nd_set_link() to return a __must_check-marked >>> errno, change callers to handle errors appropriately. >>> >>> Or something totally different ;) But along those lines? >> >> Note that XFS and possibly other filesystem don't store the NULL >> termination on disk. > > Note that ext2, for example, also only writes the string bytes without > any NULLs. It only happen to be zero padded because any last-page is zero-padded > from i_size to end of page. > >> So having a follow_link interface that uses a >> counted string would be a nice little optimization for the XFS >> follow_link / readlink implementation. But I'm not really sure it's >> worth complicating the VFS for that little gem. > > The inode's i_size already holds the string count so at the higher level > we have that information. But I'm convinced, nd_set_link() should receive > a new max_len, all users should be changed as a matter of code audit. > nd_set_link() should then proceed to truncate the string at that length > unconditionally no need for error returns. I've looked at a few alternative options: scanning for NULLs, NULL-terminating in nd_set_link, NULL-terminating in the FS code (where it is necessary and not already being done), and passing the length around explicitly. NULL-terminating is definitely cleaner and easier than scanning. Unfortunately, as Christoph indicated, passing the length around explicitly does rather complicate the code. So the question is whether to NULL-terminate in nd_set_link or earlier in the FS code. Having tried both options, I'm inclined to do it in the FS code and leave nd_set_link as it is. Many of the filesystems already take pains to ensure the links are NULL-terminated and the minimal change of fixing the others seems the safest option. However, this way we won't solve things for all filesystems for all time, as Andrew wanted. I'll post my preferred patches shortly, but if anyone would like to see what the full nd_set_link change would look like let me know and I'll post them for comparison. FYI, here are the diffstats for the two options: Terminating in FS code: fs/9p/vfs_inode.c | 5 +++-- fs/befs/linuxvfs.c | 5 ++++- fs/ecryptfs/inode.c | 3 ++- fs/ext2/symlink.c | 4 +++- fs/ext3/symlink.c | 4 +++- fs/ext4/symlink.c | 4 +++- fs/freevxfs/vxfs_immed.c | 1 + fs/jfs/symlink.c | 2 ++ fs/namei.c | 8 ++++++-- fs/sysv/symlink.c | 4 +++- fs/ufs/symlink.c | 4 +++- 11 files changed, 33 insertions(+), 11 deletions(-) Adding length param and terminating in nd_set_link (but not removing all the existing FS termination code): fs/9p/vfs_inode.c | 10 +++++----- fs/autofs/symlink.c | 2 +- fs/autofs4/symlink.c | 2 +- fs/befs/linuxvfs.c | 14 ++++++++++++-- fs/cifs/link.c | 8 ++------ fs/configfs/symlink.c | 4 ++-- fs/debugfs/file.c | 2 +- fs/ecryptfs/inode.c | 20 ++++++++++---------- fs/ext2/symlink.c | 2 +- fs/ext3/symlink.c | 2 +- fs/ext4/symlink.c | 2 +- fs/freevxfs/vxfs_immed.c | 2 +- fs/fuse/dir.c | 2 +- fs/jffs2/symlink.c | 2 +- fs/jfs/symlink.c | 3 ++- fs/namei.c | 11 +++++++++-- fs/nfs/symlink.c | 4 ++-- fs/proc/generic.c | 2 +- fs/smbfs/symlink.c | 8 ++++---- fs/sysfs/symlink.c | 2 +- fs/sysv/symlink.c | 3 ++- fs/ubifs/file.c | 2 +- fs/ufs/symlink.c | 2 +- fs/xfs/linux-2.6/xfs_iops.c | 4 ++-- include/linux/namei.h | 4 +++- mm/shmem.c | 4 ++-- 26 files changed, 70 insertions(+), 53 deletions(-) Cheers, Duane. -- "I never could learn to drink that blood and call it wine" - Bob Dylan -- 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/