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=-3.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no 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 7855FC433FE for ; Tue, 8 Dec 2020 22:23:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 48EAD239FD for ; Tue, 8 Dec 2020 22:23:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731129AbgLHWXy (ORCPT ); Tue, 8 Dec 2020 17:23:54 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57130 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730450AbgLHWXy (ORCPT ); Tue, 8 Dec 2020 17:23:54 -0500 Received: from mail-ej1-x642.google.com (mail-ej1-x642.google.com [IPv6:2a00:1450:4864:20::642]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A8CBFC0613CF for ; Tue, 8 Dec 2020 14:23:13 -0800 (PST) Received: by mail-ej1-x642.google.com with SMTP id qw4so66974ejb.12 for ; Tue, 08 Dec 2020 14:23:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=vDvIgwM3KYUr0vGzGaIWWHLyAec1spjaULYt1zzdT4s=; b=lcCLQiHgKOnWsKLetWZmR5QtSD6blr1zwiw554WVg0AiNLsDu/kt6/L2pZGQeRHIlR sk4AVKxRad9oVAN0P4xpOslW8DxPRoB5wUGIl/7BR8VDXluBCoCy3GltHE+FXzPRrQxR wQej/D6bXtMRk92njXQqKJZQH8etZbXv83JEtSOT5th+HgexH+PpCMk0aUGWuYCayWqM vOXlOUNv+ppF55wtH9HFHRDVUrwvk/2TbLHkv8ldtjRRWc/2AULYvbscWV+jss9n54tu WpKpNOLYTePYoJy7ePGx930oJcVthHof02l6Ltv8q8N765mB7nbm99dZ7SnDj/JyCKdm eCNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vDvIgwM3KYUr0vGzGaIWWHLyAec1spjaULYt1zzdT4s=; b=WWL9X9PSt9j9pLleQZ1HTAHTpSFyMrYRdBUc0Mb7wwCxQGXExkk9xh/JKve3OI/oyR ZyEM6fpbr22Ztz2y8LfaZvwkGqd3iQ487YVEgrWuQrEF427zFXrB+fA5uuvNUSccjbTu c2BPwxC0UhgWkj+6Ob0WlY5p3TPSrmk1f2FCYz8zGqMPP0tcjeWLsCl5OvF3DMqZziAo wyG+GHY58Rh98KvGQTHrioEfpMT9fg3gXHvRbxZXMSGIjW6iDW43+89DPaaoH0fmGb4h Xi/lVhCE6zYbO9Pldt20RVm7Jsvvs92dyXQ7AqrmCGrw2njn23apsKbF8qZr48XgwcjN EeSw== X-Gm-Message-State: AOAM533/PCZmj30CjPm8mGT6I/13Ktm0vGlOHxzUh+AU9g0h9pQ6Hf1G hJV/HL9rcdsIgKZcZl5WiQW0s1RwJFs0hgWDhNFmGA== X-Google-Smtp-Source: ABdhPJz7hpkHYZQB8gPmkyjxkrTY7+BAotTEDsjnew5emDltxEq0pnKaMyyY9J6ssfhQwUmQYIos3XCw3hu+XH++d88= X-Received: by 2002:a17:906:edb2:: with SMTP id sa18mr23993748ejb.264.1607466192389; Tue, 08 Dec 2020 14:23:12 -0800 (PST) MIME-Version: 1.0 References: <20201207225703.2033611-1-ira.weiny@intel.com> <20201207225703.2033611-3-ira.weiny@intel.com> <20201207232649.GD7338@casper.infradead.org> <20201207234008.GE7338@casper.infradead.org> <20201208213255.GO1563847@iweiny-DESK2.sc.intel.com> <20201208215028.GK7338@casper.infradead.org> In-Reply-To: <20201208215028.GK7338@casper.infradead.org> From: Dan Williams Date: Tue, 8 Dec 2020 14:23:10 -0800 Message-ID: Subject: Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core To: Matthew Wilcox Cc: Ira Weiny , Thomas Gleixner , Andrew Morton , Dave Hansen , Christoph Hellwig , Al Viro , Eric Biggers , Joonas Lahtinen , Linux Kernel Mailing List , linux-fsdevel Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 8, 2020 at 1:51 PM Matthew Wilcox wrote: > > On Tue, Dec 08, 2020 at 01:32:55PM -0800, Ira Weiny wrote: > > On Mon, Dec 07, 2020 at 03:49:55PM -0800, Dan Williams wrote: > > > On Mon, Dec 7, 2020 at 3:40 PM Matthew Wilcox wrote: > > > > > > > > On Mon, Dec 07, 2020 at 03:34:44PM -0800, Dan Williams wrote: > > > > > On Mon, Dec 7, 2020 at 3:27 PM Matthew Wilcox wrote: > > > > > > > > > > > > On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote: > > > > > > > +static inline void memcpy_page(struct page *dst_page, size_t dst_off, > > > > > > > + struct page *src_page, size_t src_off, > > > > > > > + size_t len) > > > > > > > +{ > > > > > > > + char *dst = kmap_local_page(dst_page); > > > > > > > + char *src = kmap_local_page(src_page); > > > > > > > > > > > > I appreciate you've only moved these, but please add: > > > > > > > > > > > > BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE); > > > > > > > > > > I imagine it's not outside the realm of possibility that some driver > > > > > on CONFIG_HIGHMEM=n is violating this assumption and getting away with > > > > > it because kmap_atomic() of contiguous pages "just works (TM)". > > > > > Shouldn't this WARN rather than BUG so that the user can report the > > > > > buggy driver and not have a dead system? > > > > > > > > As opposed to (on a HIGHMEM=y system) silently corrupting data that > > > > is on the next page of memory? > > > > > > Wouldn't it fault in HIGHMEM=y case? I guess not necessarily... > > > > > > > I suppose ideally ... > > > > > > > > if (WARN_ON(dst_off + len > PAGE_SIZE)) > > > > len = PAGE_SIZE - dst_off; > > > > if (WARN_ON(src_off + len > PAGE_SIZE)) > > > > len = PAGE_SIZE - src_off; > > > > > > > > and then we just truncate the data of the offending caller instead of > > > > corrupting innocent data that happens to be adjacent. Although that's > > > > not ideal either ... I dunno, what's the least bad poison to drink here? > > > > > > Right, if the driver was relying on "corruption" for correct operation. > > > > > > If corruption actual were happening in practice wouldn't there have > > > been screams by now? Again, not necessarily... > > > > > > At least with just plain WARN the kernel will start screaming on the > > > user's behalf, and if it worked before it will keep working. > > > > So I decided to just sleep on this because I was recently told to not introduce > > new WARN_ON's[1] > > > > I don't think that truncating len is worth the effort. The conversions being > > done should all 'work' At least corrupting users data in the same way as it > > used to... ;-) I'm ok with adding the WARN_ON's and I have modified the patch > > to do so while I work through the 0-day issues. (not sure what is going on > > there.) > > > > However, are we ok with adding the WARN_ON's given what Greg KH told me? This > > is a bit more critical than the PKS API in that it could result in corrupt > > data. > > zero_user_segments contains: > > BUG_ON(end1 > page_size(page) || end2 > page_size(page)); > > These should be consistent. I think we've demonstrated that there is > no good option here. True, but these helpers are being deployed to many new locations where they were not used before.