From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E066EC07EBF for ; Fri, 18 Jan 2019 20:02:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AB4952086D for ; Fri, 18 Jan 2019 20:02:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729342AbfARUCU (ORCPT ); Fri, 18 Jan 2019 15:02:20 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:56012 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729311AbfARUCU (ORCPT ); Fri, 18 Jan 2019 15:02:20 -0500 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.91 #2 (Red Hat Linux)) id 1gkaLK-0006KC-Ag; Fri, 18 Jan 2019 20:02:18 +0000 Date: Fri, 18 Jan 2019 20:02:18 +0000 From: Al Viro To: Christian Brauner Cc: Greg Kroah-Hartman , linux-fsdevel@vger.kernel.org Subject: Re: [heads-up] binderfs bugs galore... Message-ID: <20190118200217.GY2217@ZenIV.linux.org.uk> References: <20190117224011.GW2217@ZenIV.linux.org.uk> <20190118075712.4i5sdgtf4n3jdjpc@brauner.io> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190118075712.4i5sdgtf4n3jdjpc@brauner.io> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On Fri, Jan 18, 2019 at 08:57:14AM +0100, Christian Brauner wrote: > since we want to have each binderfs mount to be a new instance. > That means binderfs_set_super() and binderfs_test_super are gone. > So there's no weird mucking with sget_userns() and s_fs_info anymore > going on in binderfs_mount(). > That means your points from 5) to 9) should be taken care of. > > I have already a patch that remove unnecessary checks and an invalid > comment that you pointed out int 10) to 12). > > I'm currently fixing the double put issues 1) to 5) based on your > suggestions from your private mail. Thanks for that! OK... As for the cleanups on failure exits, the basic idea is to (re)use the cleanups ->kill_sb() has to do anyway. And AFAICS for binderfs it's as simple as * grab ipcns reference right after you've allocated ->s_fs_info and stick it in there right away. * make all failure exits in binderfs_fill_super() plain returns * make binderfs_kill_super() pick ->s_fs_info, do kill_litter_super(), then, if info wasn't NULL, drop ipcns ref and free info. All there is to it... We used to have tons of code duplication between the failure exits of mount and normal behaviour on umount; that caused a lot of rarely tested boilerplate code. One can still follow that model (stick the teardown on umount into ->put_super(), duplicate its parts on failure exits in ->mount()), but it can be more convenient to use an explicit ->kill_sb() instead. That's the reason why ->kill_sb() is always called for anything that made it out of sget(), whether fully set up or not. You still need to be careful about doing initialization and teardown in compatible orders, but usually that's not hard to do. Sure, it's not guaranteed to turn all failure exits into plain returns (e.g. if one does a temporary allocation that is freed in the end on setup, ->kill_sb() won't be freeing it, so any failure exits will have to take care of that), but the amount and complexity of cleanups is less that way. The same considerations, BTW, had been the reason for calling conventions of d_make_root() - simpler cleanup on failures that way...