From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f193.google.com ([209.85.192.193]:43656 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755052AbeFNMiv (ORCPT ); Thu, 14 Jun 2018 08:38:51 -0400 Received: by mail-pf0-f193.google.com with SMTP id y8-v6so3180444pfm.10 for ; Thu, 14 Jun 2018 05:38:51 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1525862104-3407-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp> <20180509160658.c37bef542a8ee5245a13917b@linux-foundation.org> <201805092346.w49NkINl045657@www262.sakura.ne.jp> <20180509165321.3b2b1313fde0f007c1a5a015@linux-foundation.org> <9ef86114-02d6-b243-203d-fbbdab95a6fa@I-love.SAKURA.ne.jp> From: Dmitry Vyukov Date: Thu, 14 Jun 2018 14:38:30 +0200 Message-ID: Subject: Re: [PATCH] bfs: add sanity check at bfs_fill_super(). To: Tigran Aivazian Cc: Tetsuo Handa , Andrew Morton , linux-fsdevel , syzbot , syzkaller-bugs Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Jun 14, 2018 at 2:23 PM, Tigran Aivazian wrote: > Hello Dmitry, > > Historically, Unix mount(2) (and mount(8)) are normally privileged > unless the filesystem is "trusted" by sysadmin who recorded this fact > in the corresponding entry in /etc/fstab (see fstab(5)). I agree that > the filesystem mounting code should perform enough validation on the > superblock to avoid crashing on an invalid filesystem image. But I > disagree that some specific filesystem (and least of all BFS!) should > modify the panic_on_warn semantics and replace it with its own private > warning message on kernel memory allocation failures. This is not a general memory allocation failure. Nothing is printed on memory allocation failures. This is a very specific case of memory allocation failure, namely when kernel code asks for too large block, such large that it can't be possible allocated regardless of amount of available memory. For allocations with user-controllable size kernel code either needs to use __GFP_NOWARN, or (better) impose own limits such that it can always be allocated with kmalloc. Consider, currently we can have a bfs image that works fine on one kernel, but fails to mount on another just because it happens so that one could allocate 4MB with kmalloc, but another can't (different allocator/different settings/different kernel revision). This looks like pretty unfortunate property for persistent data formats in general. > Kind regards, > Tigran > > On 13 June 2018 at 17:00, Dmitry Vyukov wrote: >> On Wed, Jun 13, 2018 at 3:49 PM, Tigran Aivazian >> wrote: >>> Having read the discussion carefully, I personally prefer to ignore >>> the fix as invalid, because mounting a filesystem image is a >>> privileged operation and if attempting to mount a corrupted image >>> causes a panic, that is no big deal, imho. >> >> Even if not a big deal, but still a bug? >> >> Also note that this is kind a chicken and egg problem. The only reason >> why these operations are privileged is that we have bugs and we are >> not fixing them. >> >> Also keep this in mind whenever you insert anything into usb and >> automount if turned on ;) You are basically giving permission to that >> thing to do everything with the machine. Or when you mount any image >> not created by you with trusted tools that you build yourself from >> trusted sources with a trusted compiler. >> >> Also keep in mind Android and other systems where root is not a trusted entity. >> >> >> >>> On 13 June 2018 at 14:33, Tetsuo Handa >>> wrote: >>>> On 2018/05/10 8:53, Andrew Morton wrote: >>>>> On Thu, 10 May 2018 08:46:18 +0900 Tetsuo Handa wrote: >>>>> >>>>>>> page-allocation-fauilure warning and a nice backtrace, etc. Why >>>>>>> suppress all of that and add our custom warning instead? >>>>>> >>>>>> the intent of this patch is to avoid panic() by panic_on_warn == 1 >>>>>> due to hitting >>>>>> >>>>>> struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags) >>>>>> { >>>>>> unsigned int index; >>>>>> >>>>>> if (unlikely(size > KMALLOC_MAX_SIZE)) { >>>>>> WARN_ON_ONCE(!(flags & __GFP_NOWARN)); /* <= this line */ >>>>>> return NULL; >>>>>> } >>>>>> >>>>>> when size to allocate is controlled by the filesystem image. >>>>> >>>>> Well, the same could happen with many many memory-allocation sites. >>>>> What's special about BFS? If someone sets panic_on_warn=1 then >>>>> presumably this panic is the behaviour they wanted in this case. >>>>> >>>> >>>> Tigran, this patch is stalling. Do we want to apply this? Or, ignore as invalid? >>>> >>>> errors=panic mount option for ext4 case was ignored as invalid. >>>> http://lkml.kernel.org/r/CACT4Y+Z+2YW_VALJzzQr6hLsviA=dXk3iFqwVf+P5zqojeC9Zg@mail.gmail.com >>>> >>>> But I prefer avoiding crashes if we can fix it. >>> >>> -- >>> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. >>> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. >>> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/CAK%2B_RLko_OCepN4FCmsaQPAKkt9JNGe8pNRK7SO-onhw5zCneA%40mail.gmail.com. >>> For more options, visit https://groups.google.com/d/optout.