From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:50253 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727749AbeIFMxO (ORCPT ); Thu, 6 Sep 2018 08:53:14 -0400 Received: by mail-wm0-f67.google.com with SMTP id s12-v6so10500137wmc.0 for ; Thu, 06 Sep 2018 01:18:57 -0700 (PDT) Date: Thu, 6 Sep 2018 10:18:54 +0200 From: Carlos Maiolino To: Matthew Wilcox Cc: linux-fsdevel@vger.kernel.org, hch@lst.de, sandeen@redhat.com, david@fromorbit.com Subject: Re: [PATCH 2/2] Use fiemap internal infra-structure to handle FIBMAP Message-ID: <20180906081854.hsa3yijwd52yh5y5@odin.usersys.redhat.com> References: <20180905135748.30098-1-cmaiolino@redhat.com> <20180905135748.30098-3-cmaiolino@redhat.com> <20180905143147.GC3729@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180905143147.GC3729@bombadil.infradead.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Hi Matthew. On Wed, Sep 05, 2018 at 07:31:47AM -0700, Matthew Wilcox wrote: > On Wed, Sep 05, 2018 at 03:57:48PM +0200, Carlos Maiolino wrote: > > + if (inode->i_op->fiemap) { > > + fextent.fe_logical = 0; > > + fextent.fe_physical = 0; > > + fieinfo.fi_flags = FIEMAP_KERNEL_FIBMAP; > > + fieinfo.fi_extents_max = 1; > > + fieinfo.fi_extents_start = (__force struct fiemap_extent __user *) &fextent; > > + > > + error = inode->i_op->fiemap(inode, &fieinfo, start, 1); > > You'd have to play games with set_fs() and friends if you want to do this. > The fiemap implementation is going to access fi_extents_start with a call > to copy_to_user() and for machines with a 4G/4G split, you need that > address to be interpreted as kernel space, not user space. > Thanks for the review, It's the very first time I try to 'mix' __user pointers with internal-only kernel code, I did some research on how other parts of kernel handles it, so I'm kind of stepping on eggs here. I do have a question about your point though: > See fiemap_fill_next_extent(): > > struct fiemap_extent __user *dest = fieinfo->fi_extents_start; > ... > if (copy_to_user(dest, &extent, sizeof(extent))) > return -EFAULT; > Yes, I noticed it, and, that's why I added the FIEMAP_KERNEL_FIBMAP flag. This is the changes I did to fiemap_fill_next_extent(): @@ -117,8 +117,14 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical, extent.fe_flags = flags; dest += fieinfo->fi_extents_mapped; - if (copy_to_user(dest, &extent, sizeof(extent))) - return -EFAULT; + + if (fieinfo->fi_flags & FIEMAP_KERNEL_FIBMAP) { + memcpy((__force struct fiemap_extent *)dest, &extent, + sizeof(extent)); + } else { + if (copy_to_user(dest, &extent, sizeof(extent))) + return -EFAULT; + } So, my patch is going to use memcpy() directly, instead of copy_to_user(), if the request comes from the bmap() function, other than that, I'm not really sure I understand your point here. -- Carlos