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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id CD43DC4167D for ; Mon, 28 Nov 2022 08:56:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0BFA86B0074; Mon, 28 Nov 2022 03:56:53 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 06F506B0075; Mon, 28 Nov 2022 03:56:53 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E525C6B0078; Mon, 28 Nov 2022 03:56:52 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id D34D16B0074 for ; Mon, 28 Nov 2022 03:56:52 -0500 (EST) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 9DCE41C07BC for ; Mon, 28 Nov 2022 08:56:52 +0000 (UTC) X-FDA: 80182245864.08.3AA0610 Received: from mail-ej1-f45.google.com (mail-ej1-f45.google.com [209.85.218.45]) by imf30.hostedemail.com (Postfix) with ESMTP id 16BF280009 for ; Mon, 28 Nov 2022 08:56:51 +0000 (UTC) Received: by mail-ej1-f45.google.com with SMTP id td2so9908537ejc.5 for ; Mon, 28 Nov 2022 00:56:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=GayucSwjXWyD6TV/kxm2u2wBavJA3ia7DZeWYkCZKOw=; b=by6nBjiXr9NZh8kyPhKz/QR5ZDh+Mr6rd8SZDiWMrpQwfDjq50XZ5M8w7bj1PvwuXn kLkTx7A4ur7lbkYVx6axrDESX+rGk1rmxp2uzRJp0E85uaCMYFTHvsVEip/Ap9RC3SiD fpJiyTPqz1ccLG964MJ0C8yB5VPBwkMbhooH4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=GayucSwjXWyD6TV/kxm2u2wBavJA3ia7DZeWYkCZKOw=; b=SOc4ohwYYx+xLahHaAJNkvzm7JQMlcrGYOc5X2+PDeLQ4ysRpwh0/62terLpAaN7KQ sXQ3caZsVk6c2t2uF56XJJiQPGUAitU5ZsC0JqyTBvXmWLd1mZB23eaxOIR8N0zp1lPx 28XSZlXOxHvKS3FIQu7Arwfc6IhqbSdRxbg47+G0pPcYRF0WvR3S6Agjz9rfyHVOBSDc duhdbIIW92zNeYg15ghjpT1lYJGoErz+DcMFpw3BEu3tJq6zdTh4ETnNkHM4mNgZyDc9 rUfLwYHFvTBdlb0f3NKhnNHolj+VRK/lTB0MQPA+ibwQy6m9BThY/v0oonjwSBcUGE3k x/xA== X-Gm-Message-State: ANoB5pkkqv3a0bb2T82mZm6qDQ9HvtXrnwgZqhubcFMZ5nLjukIlFUED J75wbIjpKPyprgy1zuYD7feqt+4u0GS2aw== X-Google-Smtp-Source: AA0mqf6XSsanPf0VGps2Rud4vGZnagHrRRwxg4/iSeM/RlF1OfZyvvd2Ezo4+qrUgSODSNcO4/YLcQ== X-Received: by 2002:a17:906:9c89:b0:7c0:7ab4:3715 with SMTP id fj9-20020a1709069c8900b007c07ab43715mr1332639ejc.343.1669625810311; Mon, 28 Nov 2022 00:56:50 -0800 (PST) Received: from mail-ej1-f53.google.com (mail-ej1-f53.google.com. [209.85.218.53]) by smtp.gmail.com with ESMTPSA id vq2-20020a170907a4c200b007aef930360asm4700353ejc.59.2022.11.28.00.56.49 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 28 Nov 2022 00:56:49 -0800 (PST) Received: by mail-ej1-f53.google.com with SMTP id ha10so24060399ejb.3 for ; Mon, 28 Nov 2022 00:56:49 -0800 (PST) X-Received: by 2002:a17:906:a889:b0:7ad:bd02:f491 with SMTP id ha9-20020a170906a88900b007adbd02f491mr43999728ejb.703.1669625809184; Mon, 28 Nov 2022 00:56:49 -0800 (PST) MIME-Version: 1.0 References: <9d559d56-b042-9955-f7d1-20c50c2c8e03@xs4all.nl> In-Reply-To: <9d559d56-b042-9955-f7d1-20c50c2c8e03@xs4all.nl> From: Tomasz Figa Date: Mon, 28 Nov 2022 17:56:37 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH RESEND] media: videobuf2: revert "get_userptr: buffers are always writable" To: Hans Verkuil Cc: Linux Media Mailing List , Hirokazu Honda , David Hildenbrand , Andrew Morton , linux-mm@kvack.org, Linux Kernel Content-Type: text/plain; charset="UTF-8" ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669625812; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=GayucSwjXWyD6TV/kxm2u2wBavJA3ia7DZeWYkCZKOw=; b=GhAR1szBkt1e82kJ5ka4YTbvRCnRPGMpWsO3+/bTqW4uARGffIzaash0X2gZXE+8lkHCaD CXqIYhj9Ps+kXsz14KjK0hDOKDL7OFp0l2X3FxZbc8LEiEgDlVT7WoyTf/X6ypy/BfGgTZ JVfUNwnnBwqyM08POkrlNMRHeRTVFjo= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=by6nBjiX; spf=pass (imf30.hostedemail.com: domain of tfiga@chromium.org designates 209.85.218.45 as permitted sender) smtp.mailfrom=tfiga@chromium.org; dmarc=pass (policy=none) header.from=chromium.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669625812; a=rsa-sha256; cv=none; b=pSpo32ECdhl9IwpdFs7AP/sMev1SbAVZmEmRp+seAeKB2TgraXQhQ1Oyvim3Va8u/F62oX NUawCwllJJF9JoO7iff1tceCRi9rRoGqiDRB3qwvaQaQKxiXAzVvlmv1hmUm8EwLSdncVc BHBIzsvgYmdcWXQEcOFXTbv0t/fqNek= X-Stat-Signature: 7xi1147t45ifah7dnjhkhazkqb7f7fxc X-Rspamd-Queue-Id: 16BF280009 Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=by6nBjiX; spf=pass (imf30.hostedemail.com: domain of tfiga@chromium.org designates 209.85.218.45 as permitted sender) smtp.mailfrom=tfiga@chromium.org; dmarc=pass (policy=none) header.from=chromium.org X-Rspam-User: X-Rspamd-Server: rspam05 X-HE-Tag: 1669625811-654713 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Nov 28, 2022 at 5:24 PM Hans Verkuil wrote: > > Commit 707947247e95 ("media: videobuf2-vmalloc: get_userptr: buffers are > always writable") caused problems in a corner case (passing read-only > shmem memory as a userptr). So revert this patch. > > The original problem for which that commit was originally made is > something that I could not reproduce after reverting it. So just go > back to the way it was for many years, and if problems arise in > the future, then another approach should be taken to resolve it. > > This patch is based on a patch from Hirokazu. > > Fixes: 707947247e95 ("media: videobuf2-vmalloc: get_userptr: buffers are always writable") > Signed-off-by: Hirokazu Honda > Signed-off-by: Hans Verkuil > --- > CCed also to Andrew, linux-mm and linux-kernel. This patch is meant to be > first in David Hildenbrand's 'remove FOLL_FORCE' series to ensure that it > will be easy to backport this fix to older kernels without introducing new > merge conflicts. > > Hans > --- > drivers/media/common/videobuf2/frame_vector.c | 10 +++++++--- > drivers/media/common/videobuf2/videobuf2-dma-contig.c | 3 ++- > drivers/media/common/videobuf2/videobuf2-dma-sg.c | 4 +++- > drivers/media/common/videobuf2/videobuf2-memops.c | 6 ++++-- > drivers/media/common/videobuf2/videobuf2-vmalloc.c | 4 +++- > include/media/frame_vector.h | 2 +- > include/media/videobuf2-memops.h | 3 ++- > 7 files changed, 22 insertions(+), 10 deletions(-) > Thanks! Acked-by: Tomasz Figa Best regards, Tomasz > diff --git a/drivers/media/common/videobuf2/frame_vector.c b/drivers/media/common/videobuf2/frame_vector.c > index 542dde9d2609..aad72640f055 100644 > --- a/drivers/media/common/videobuf2/frame_vector.c > +++ b/drivers/media/common/videobuf2/frame_vector.c > @@ -14,6 +14,7 @@ > * get_vaddr_frames() - map virtual addresses to pfns > * @start: starting user address > * @nr_frames: number of pages / pfns from start to map > + * @write: the mapped address has write permission > * @vec: structure which receives pages / pfns of the addresses mapped. > * It should have space for at least nr_frames entries. > * > @@ -32,7 +33,7 @@ > * > * This function takes care of grabbing mmap_lock as necessary. > */ > -int get_vaddr_frames(unsigned long start, unsigned int nr_frames, > +int get_vaddr_frames(unsigned long start, unsigned int nr_frames, bool write, > struct frame_vector *vec) > { > struct mm_struct *mm = current->mm; > @@ -40,6 +41,7 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, > int ret_pin_user_pages_fast = 0; > int ret = 0; > int err; > + unsigned int gup_flags = FOLL_FORCE | FOLL_LONGTERM; > > if (nr_frames == 0) > return 0; > @@ -49,8 +51,10 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, > > start = untagged_addr(start); > > - ret = pin_user_pages_fast(start, nr_frames, > - FOLL_FORCE | FOLL_WRITE | FOLL_LONGTERM, > + if (write) > + gup_flags |= FOLL_WRITE; > + > + ret = pin_user_pages_fast(start, nr_frames, gup_flags, > (struct page **)(vec->ptrs)); > if (ret > 0) { > vec->got_ref = true; > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > index 678b359717c4..8e55468cb60d 100644 > --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c > +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > @@ -603,7 +603,8 @@ static void *vb2_dc_get_userptr(struct vb2_buffer *vb, struct device *dev, > buf->vb = vb; > > offset = lower_32_bits(offset_in_page(vaddr)); > - vec = vb2_create_framevec(vaddr, size); > + vec = vb2_create_framevec(vaddr, size, buf->dma_dir == DMA_FROM_DEVICE || > + buf->dma_dir == DMA_BIDIRECTIONAL); > if (IS_ERR(vec)) { > ret = PTR_ERR(vec); > goto fail_buf; > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > index fa69158a65b1..099693e42bc6 100644 > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > @@ -241,7 +241,9 @@ static void *vb2_dma_sg_get_userptr(struct vb2_buffer *vb, struct device *dev, > buf->size = size; > buf->dma_sgt = &buf->sg_table; > buf->vb = vb; > - vec = vb2_create_framevec(vaddr, size); > + vec = vb2_create_framevec(vaddr, size, > + buf->dma_dir == DMA_FROM_DEVICE || > + buf->dma_dir == DMA_BIDIRECTIONAL); > if (IS_ERR(vec)) > goto userptr_fail_pfnvec; > buf->vec = vec; > diff --git a/drivers/media/common/videobuf2/videobuf2-memops.c b/drivers/media/common/videobuf2/videobuf2-memops.c > index 9dd6c27162f4..f9a4ec44422e 100644 > --- a/drivers/media/common/videobuf2/videobuf2-memops.c > +++ b/drivers/media/common/videobuf2/videobuf2-memops.c > @@ -26,6 +26,7 @@ > * vb2_create_framevec() - map virtual addresses to pfns > * @start: Virtual user address where we start mapping > * @length: Length of a range to map > + * @write: Should we map for writing into the area > * > * This function allocates and fills in a vector with pfns corresponding to > * virtual address range passed in arguments. If pfns have corresponding pages, > @@ -34,7 +35,8 @@ > * failure. Returned vector needs to be freed via vb2_destroy_pfnvec(). > */ > struct frame_vector *vb2_create_framevec(unsigned long start, > - unsigned long length) > + unsigned long length, > + bool write) > { > int ret; > unsigned long first, last; > @@ -47,7 +49,7 @@ struct frame_vector *vb2_create_framevec(unsigned long start, > vec = frame_vector_create(nr); > if (!vec) > return ERR_PTR(-ENOMEM); > - ret = get_vaddr_frames(start & PAGE_MASK, nr, vec); > + ret = get_vaddr_frames(start & PAGE_MASK, nr, write, vec); > if (ret < 0) > goto out_destroy; > /* We accept only complete set of PFNs */ > diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c > index 948152f1596b..67d0b89e701b 100644 > --- a/drivers/media/common/videobuf2/videobuf2-vmalloc.c > +++ b/drivers/media/common/videobuf2/videobuf2-vmalloc.c > @@ -85,7 +85,9 @@ static void *vb2_vmalloc_get_userptr(struct vb2_buffer *vb, struct device *dev, > buf->dma_dir = vb->vb2_queue->dma_dir; > offset = vaddr & ~PAGE_MASK; > buf->size = size; > - vec = vb2_create_framevec(vaddr, size); > + vec = vb2_create_framevec(vaddr, size, > + buf->dma_dir == DMA_FROM_DEVICE || > + buf->dma_dir == DMA_BIDIRECTIONAL); > if (IS_ERR(vec)) { > ret = PTR_ERR(vec); > goto fail_pfnvec_create; > diff --git a/include/media/frame_vector.h b/include/media/frame_vector.h > index bfed1710dc24..541c71a2c7be 100644 > --- a/include/media/frame_vector.h > +++ b/include/media/frame_vector.h > @@ -16,7 +16,7 @@ struct frame_vector { > struct frame_vector *frame_vector_create(unsigned int nr_frames); > void frame_vector_destroy(struct frame_vector *vec); > int get_vaddr_frames(unsigned long start, unsigned int nr_pfns, > - struct frame_vector *vec); > + bool write, struct frame_vector *vec); > void put_vaddr_frames(struct frame_vector *vec); > int frame_vector_to_pages(struct frame_vector *vec); > void frame_vector_to_pfns(struct frame_vector *vec); > diff --git a/include/media/videobuf2-memops.h b/include/media/videobuf2-memops.h > index cd4a46331531..4b5b84f93538 100644 > --- a/include/media/videobuf2-memops.h > +++ b/include/media/videobuf2-memops.h > @@ -34,7 +34,8 @@ struct vb2_vmarea_handler { > extern const struct vm_operations_struct vb2_common_vm_ops; > > struct frame_vector *vb2_create_framevec(unsigned long start, > - unsigned long length); > + unsigned long length, > + bool write); > void vb2_destroy_framevec(struct frame_vector *vec); > > #endif > -- > 2.35.1 >