| Message ID | 1429553588-24764-1-git-send-email-viro@ZenIV.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Apr 20, 2015, at 12:12 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote: > > From: Al Viro <viro@zeniv.linux.org.uk> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Al, the patch itself looks good, thanks. However, if this is applied at the start of the series it could allow tests to easily cause a stack overflow during a bisection (I don't think users would see a kernel in the middle of the series). Could this be converted over to checking nd->link_count along with the [02/24] patch until closer to the end of the series when the recursion has been removed? It isn't fatal if that doesn't happen, since this whole series should land at one time and the chance of testing Lustre symlinks right in the middle of the series is low, just something I thought when reviewing the patch. Cheers, Andreas > --- > drivers/staging/lustre/lustre/llite/symlink.c | 15 +++------------ > 1 file changed, 3 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/lustre/lustre/llite/symlink.c b/drivers/staging/lustre/lustre/llite/symlink.c > index 3711e67..0615f86 100644 > --- a/drivers/staging/lustre/lustre/llite/symlink.c > +++ b/drivers/staging/lustre/lustre/llite/symlink.c > @@ -126,18 +126,9 @@ static void *ll_follow_link(struct dentry *dentry, struct nameidata *nd) > char *symname = NULL; > > CDEBUG(D_VFSTRACE, "VFS Op\n"); > - /* Limit the recursive symlink depth to 5 instead of default > - * 8 links when kernel has 4k stack to prevent stack overflow. > - * For 8k stacks we need to limit it to 7 for local servers. */ > - if (THREAD_SIZE < 8192 && current->link_count >= 6) { > - rc = -ELOOP; > - } else if (THREAD_SIZE == 8192 && current->link_count >= 8) { > - rc = -ELOOP; > - } else { > - ll_inode_size_lock(inode); > - rc = ll_readlink_internal(inode, &request, &symname); > - ll_inode_size_unlock(inode); > - } > + ll_inode_size_lock(inode); > + rc = ll_readlink_internal(inode, &request, &symname); > + ll_inode_size_unlock(inode); > if (rc) { > ptlrpc_req_finished(request); > request = NULL; > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 20, 2015 at 01:08:16PM -0600, Andreas Dilger wrote: > On Apr 20, 2015, at 12:12 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote: > > > > From: Al Viro <viro@zeniv.linux.org.uk> > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > Al, the patch itself looks good, thanks. > > However, if this is applied at the start of the series it could > allow tests to easily cause a stack overflow during a bisection (I > don't think users would see a kernel in the middle of the series). > > Could this be converted over to checking nd->link_count along with > the [02/24] patch until closer to the end of the series when the > recursion has been removed? Er... You do realize that struct nameidata is opaque for anything outside of fs/namei.c and has been that way for a while now? Sure, we can export a helper that would return that and rip it out in the end of the series, but... > It isn't fatal if that doesn't happen, since this whole series should > land at one time and the chance of testing Lustre symlinks right > in the middle of the series is low, just something I thought when > reviewing the patch. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 20, 2015 at 08:22:53PM +0100, Al Viro wrote: > On Mon, Apr 20, 2015 at 01:08:16PM -0600, Andreas Dilger wrote: > > On Apr 20, 2015, at 12:12 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote: > > > > > > From: Al Viro <viro@zeniv.linux.org.uk> > > > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > > > Al, the patch itself looks good, thanks. > > > > However, if this is applied at the start of the series it could > > allow tests to easily cause a stack overflow during a bisection (I > > don't think users would see a kernel in the middle of the series). > > > > Could this be converted over to checking nd->link_count along with > > the [02/24] patch until closer to the end of the series when the > > recursion has been removed? > > Er... You do realize that struct nameidata is opaque for anything outside > of fs/namei.c and has been that way for a while now? Sure, we can export > a helper that would return that and rip it out in the end of the series, > but... Actually, a cleaner solution would be to reorder that bunch (1--6) past the link_path_walk() reorganization. Done and force-pushed to the same branch... -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/lustre/lustre/llite/symlink.c b/drivers/staging/lustre/lustre/llite/symlink.c index 3711e67..0615f86 100644 --- a/drivers/staging/lustre/lustre/llite/symlink.c +++ b/drivers/staging/lustre/lustre/llite/symlink.c @@ -126,18 +126,9 @@ static void *ll_follow_link(struct dentry *dentry, struct nameidata *nd) char *symname = NULL; CDEBUG(D_VFSTRACE, "VFS Op\n"); - /* Limit the recursive symlink depth to 5 instead of default - * 8 links when kernel has 4k stack to prevent stack overflow. - * For 8k stacks we need to limit it to 7 for local servers. */ - if (THREAD_SIZE < 8192 && current->link_count >= 6) { - rc = -ELOOP; - } else if (THREAD_SIZE == 8192 && current->link_count >= 8) { - rc = -ELOOP; - } else { - ll_inode_size_lock(inode); - rc = ll_readlink_internal(inode, &request, &symname); - ll_inode_size_unlock(inode); - } + ll_inode_size_lock(inode); + rc = ll_readlink_internal(inode, &request, &symname); + ll_inode_size_unlock(inode); if (rc) { ptlrpc_req_finished(request); request = NULL;