From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756704AbcJMUnq (ORCPT ); Thu, 13 Oct 2016 16:43:46 -0400 Received: from smtp-fw-9101.amazon.com ([207.171.184.25]:10550 "EHLO smtp-fw-9101.amazon.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754537AbcJMUni (ORCPT ); Thu, 13 Oct 2016 16:43:38 -0400 X-IronPort-AV: E=Sophos;i="5.31,489,1473120000"; d="scan'208";a="647160793" Subject: Re: [PATCH] namei: revert old behaviour for filename_lookup with LOOKUP_PARENT flag To: Al Viro , Christoph Hellwig References: <1476388731-24053-1-git-send-email-vineethp@amazon.com> <20161013200904.GA23139@infradead.org> <20161013202622.GS19539@ZenIV.linux.org.uk> CC: , , , From: Vineeth Remanan Pillai Message-ID: <5d487c39-25e9-2903-1fd9-ca6d870c0e7b@amazon.com> Date: Thu, 13 Oct 2016 13:41:11 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20161013202622.GS19539@ZenIV.linux.org.uk> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.43.161.139] X-ClientProxiedBy: EX13D15UWB003.ant.amazon.com (10.43.161.138) To EX13D08UWC003.ant.amazon.com (10.43.162.21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/13/2016 01:26 PM, Al Viro wrote: > On Thu, Oct 13, 2016 at 01:09:04PM -0700, Christoph Hellwig wrote: >> On Thu, Oct 13, 2016 at 07:58:51PM +0000, Vineeth Remanan Pillai wrote: >>> filename_lookup used to return success for non-existing file when called >>> with LOOKUP_PARENT flag. This behaviour was changed with >>> commit 8bcb77fabd7c ("namei: split off filename_lookupat() >>> with LOOKUP_PARENT") >>> >>> The above patch split parent lookup functionality to a different function >>> filename_parentat and changed all calls to filename_lookup(LOOKUP_PARENT) >>> to the new function filename_parentat. But functions like kern_path which >>> passed the flags directly to filename_lookup regressed due to this. >>> >>> This patch aims to fix the regressed behaviour by calling >>> filename_parentat from filename_lookup if the flags contain LOOKUP_PARENT. >> What callers shows te problems? That's probaby were the fix need to got >> in, and even if not that's still part of a good bug report. > Out-of-tree, at a guess... Incidentally, since filename_lookup() is not > exported, it's probably kern_path() that gets used wherever it is. > Depending on the details of what's being attempted, kern_path_locked() > might or might not be the right primitive to use, but I would probably > start with checking if that's what the code in question really wants. > > Use: > // kernel_string is an kernel pointer to NUL-terminated array of char > > struct path path; > struct dentry *dentry; > > dentry = kern_path_locked(kernel_string, &path); > if (IS_ERR(dentry)) > // failed while getting the parent, or not a regular last > // component (/, ., .., /., /..) > sod off // no cleanup needed > > // now path contains the resolved parent and dentry points to the > // dentry of child, possibly negative; the last component of the > // name can be determined from dentry->d_name. Parent directory > // is locked, making sure that directory entry won't be changed > // until you are done. > > if (d_is_really_negative(dentry)) { > // parent exists, but child doesn't > } else { > // child exists > } > > // clean up: drop dentry, unlock parent, drop dentry/vfsmount of parent > dput(dentry); > inode_unlock(path.dentry->d_inode); > path_put(&path); > Yes, the use case is out-of-tree and the code snippet above depicts the use . Since kern_path_locked is also not exported, out-of-tree code used kern_path for the existence check for directories. One reference about this issue can be seen here. http://www.gossamer-threads.com/lists/linux/kernel/2459690?do=post_view_flat#2459690 We also have a customer who complained about this functionality change. I understand that there has been no API promises been made to this API. But since this is an exported function, the change in function could cause break in out-of-tree kernel code. I will rephrase the commit message to say "change in functionality" instead of regression