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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1BD4EC7EE21 for ; Sun, 23 Apr 2023 22:29:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229701AbjDWW3t (ORCPT ); Sun, 23 Apr 2023 18:29:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60570 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229516AbjDWW3r (ORCPT ); Sun, 23 Apr 2023 18:29:47 -0400 Received: from mail-pl1-x634.google.com (mail-pl1-x634.google.com [IPv6:2607:f8b0:4864:20::634]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1A5DCE7C for ; Sun, 23 Apr 2023 15:29:45 -0700 (PDT) Received: by mail-pl1-x634.google.com with SMTP id d9443c01a7336-1a68f2345c5so31830475ad.2 for ; Sun, 23 Apr 2023 15:29:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20221208.gappssmtp.com; s=20221208; t=1682288984; x=1684880984; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=WCA3zsna6vFeV798erY3+vSi/BvHgoXGHGGpFGJHBjM=; b=CSv4RH6SXo/8LhDuQi8y+f+aMDFvLryZeZLhrGCIHJ76AP0hsBy9FXFG82OCSDXpdw w8F2b+EujbdmHkHGv/Yp2eXHQ1Q2MYfO8Vu1ERFY1LXyjQpIg0lB+5VlMI/ocJwxnhdc Xr7ocs5kYhWzgrbsToVMuFqmxnhx9PMp/1Es7VJKZagcRT2c06hsYHc4SKreGkyREEPk mCg+asSla1YXUzn1ZIV7XG9Mcyje8X207NucVVd2pMhQOzQUptnWXdlOsGXi/9ALcAP/ iq+8gIFu9tNpqRG1e620R+RCQL23t/f9n43Ntbw7lzsRcOiSQ4oQmZOQImEBwjpawJpm dy5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682288984; x=1684880984; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=WCA3zsna6vFeV798erY3+vSi/BvHgoXGHGGpFGJHBjM=; b=AFXCaJFrToBKyne0n86w7aa6ryUr7dhgmVGcK+2dMyG80AjUKPufCwHUwe3k1Fu+9R cNcIU9bpa4QAhAMn/TosrdxMzK70YmFEQGBEDC9b38YYhfBa9lT2gp8rZjrk7T6QqkKM xasTIM3Z3MJpgQi4HIcMyLT2JME/lnJdJDvM15njaqX6oBGT8Ao1CXnZZTu7wNiCGs0j 6IZHM5E4w1rRlSA6NNNyBJtb3Ep7osM3H/IRryhw5a0/f2pqgPQyfHkU1c9XcnRZxBNc mSKRpUGu5GzLIa8IhoGPwSZvoqzAPgidBhag5ZJSZ1KAqa8J5HahCO+q+gKW8xcPHQLq JgVA== X-Gm-Message-State: AAQBX9fgLQpRrtOgb4KeuG0Qn24RBmyjwkqEar3q0kletztDtSmjLqpw sfj+9xaZFHyTS5zQ/s4sJtUfzQ== X-Google-Smtp-Source: AKy350ZR4n3yiJ8OSE0TQKFE8uJrXQJX3r2CnA0Tf9NWbUzvYbwag62F1wj/UfIK9SSx06miuG3AKw== X-Received: by 2002:a17:902:e849:b0:1a6:dba5:2e3e with SMTP id t9-20020a170902e84900b001a6dba52e3emr14801275plg.25.1682288984544; Sun, 23 Apr 2023 15:29:44 -0700 (PDT) Received: from dread.disaster.area (pa49-180-41-174.pa.nsw.optusnet.com.au. [49.180.41.174]) by smtp.gmail.com with ESMTPSA id bh8-20020a170902a98800b001a641ea111fsm5444609plb.112.2023.04.23.15.29.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 23 Apr 2023 15:29:44 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.92.3) (envelope-from ) id 1pqiDF-0074Ij-4U; Mon, 24 Apr 2023 08:29:41 +1000 Date: Mon, 24 Apr 2023 08:29:41 +1000 From: Dave Chinner To: Lorenzo Stoakes Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Jason Gunthorpe , Jens Axboe , Matthew Wilcox , Dennis Dalessandro , Leon Romanovsky , Christian Benvenuti , Nelson Escobar , Bernard Metzler , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Ian Rogers , Adrian Hunter , Bjorn Topel , Magnus Karlsson , Maciej Fijalkowski , Jonathan Lemon , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Christian Brauner , Richard Cochran , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , linux-fsdevel@vger.kernel.org, linux-perf-users@vger.kernel.org, netdev@vger.kernel.org, bpf@vger.kernel.org Subject: Re: [PATCH] mm/gup: disallow GUP writing to file-backed mappings by default Message-ID: <20230423222941.GR447837@dread.disaster.area> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Apr 22, 2023 at 02:37:05PM +0100, Lorenzo Stoakes wrote: > +/* > + * Writing to file-backed mappings using GUP is a fundamentally broken operation > + * as kernel write access to GUP mappings may not adhere to the semantics > + * expected by a file system. > + * > + * In most instances we disallow this broken behaviour, however there are some > + * exceptions to this enforced here. > + */ > +static inline bool can_write_file_mapping(struct vm_area_struct *vma, > + unsigned long gup_flags) > +{ > + struct file *file = vma->vm_file; > + > + /* If we aren't pinning then no problematic write can occur. */ > + if (!(gup_flags & (FOLL_GET | FOLL_PIN))) > + return true; > + > + /* Special mappings should pose no problem. */ > + if (!file) > + return true; Ok... > + > + /* Has the caller explicitly indicated this case is acceptable? */ > + if (gup_flags & FOLL_ALLOW_BROKEN_FILE_MAPPING) > + return true; > + > + /* shmem and hugetlb mappings do not have problematic semantics. */ > + return vma_is_shmem(vma) || is_file_hugepages(file); > +} This looks backwards. We only want the override to occur when the target won't otherwise allow it. i.e. This should be: if (vma_is_shmem(vma)) return true; if (is_file_hugepages(vma) return true; /* * Issue a warning only if we are allowing a write to a mapping * that does not support what we are attempting to do functionality. */ if (WARN_ON_ONCE(gup_flags & FOLL_ALLOW_BROKEN_FILE_MAPPING)) return true; return false; i.e. we only want the warning to fire when the override is triggered - indicating that the caller is actually using a file mapping in a broken way, not when it is being used on file/filesystem that actually supports file mappings in this way. > static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) > { > vm_flags_t vm_flags = vma->vm_flags; > int write = (gup_flags & FOLL_WRITE); > int foreign = (gup_flags & FOLL_REMOTE); > + bool vma_anon = vma_is_anonymous(vma); > > if (vm_flags & (VM_IO | VM_PFNMAP)) > return -EFAULT; > > - if (gup_flags & FOLL_ANON && !vma_is_anonymous(vma)) > + if ((gup_flags & FOLL_ANON) && !vma_anon) > return -EFAULT; > > if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma)) > @@ -978,6 +1008,10 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) > return -EFAULT; > > if (write) { > + if (!vma_anon && > + WARN_ON_ONCE(!can_write_file_mapping(vma, gup_flags))) > + return -EFAULT; Yeah, the warning definitely belongs in the check function when the override triggers allow broken behaviour to proceed, not when we disallow a write fault because the underlying file/filesystem does not support the operation being attempted. -Dave. -- Dave Chinner david@fromorbit.com