From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EAC7E3242B2; Sat, 22 Aug 2026 19:24:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=216.40.44.15 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787426663; cv=none; b=Rf9p0ZM8obdJmZ64lqqBJFYMGz4BrRghgTyxcBDMhtKTW+5EW8vAymhctWn3lMJJe9vR43wIU6A+onWrqTjkJkhnexaeMBlgbywlftFZvVF0uBP8FOWKAxqOKR+cRt0SPpUFcITxiFiRABevVz4Z21M9/vt99k03ZBvwoHNkk3Y= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787426663; c=relaxed/simple; bh=EnVTf8gwPDT5HYwqYo0yJy/jFSm9GDsLPStDR83Xyjo=; h=MIME-Version:Date:From:To:Cc:Message-Id:In-Reply-To:References: Subject:Content-Type; b=VcSRyCU+f6xmp9JT3tsOxTZbktWsfOucH8NZisPOniOdfGuoYGWgdahtS3eWCLygZbDtcAJwxP9wxILxPMtTYVbSSpzih2OQllppH8Jg7CCz5/Lf51mIWgYZrA2Bj+nozPlUPhRK2FxTTdKYUSTIk6Z7HSsGFZ4/S4xGQ9uhET8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=groves.net; spf=pass smtp.mailfrom=groves.net; arc=none smtp.client-ip=216.40.44.15 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=groves.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=groves.net Received: from omf09.hostedemail.com (lb01a-stub [10.200.18.249]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 709901C09D0; Sat, 22 Aug 2026 19:24:19 +0000 (UTC) Received: from [HIDDEN] (Authenticated sender: john@groves.net) by omf09.hostedemail.com (Postfix) with ESMTPA id 9797B2002A; Sat, 22 Aug 2026 19:24:17 +0000 (UTC) Received: from phl-compute-09.internal (phl-compute-09.internal [10.202.2.49]) by mailfauth.phl.internal (Postfix) with ESMTP id 0CE2FF40066; Sat, 22 Aug 2026 15:24:17 -0400 (EDT) Received: from phl-imap-02 ([10.202.2.81]) by phl-compute-09.internal (MEProxy); Sat, 22 Aug 2026 15:24:17 -0400 X-ME-Sender: X-ME-Proxy-Cause: dmFkZTEvraoE52QKgzrjVZx7y1Nh26WxfrVWVx85I0tKnFYu7wPFkalH5pSmF7ZtXe4VUL V/YGxhEfKMPG1RQ2WEl/BDCnPB87I6zqJ4aefcoSgQsNxJMU6CoWC8Twi27v30pNXO7Cx9 +w3yTS4nYyBumtAGmrjoue4GwJ+x1mhOkLk26vcGJECcNo6A8QyqD6gdF+4Cbf4GBjzKOn 3bgvC9drrdeBXNKSCGjWDW7mM/8ufJw4aZQJm92g3zdjbXtvsPkYAl1U5nGwLLEngyqStC rJsmWyvHmVV24l3NNw1/L5z3y9FB2iMGQGb7wvuzLvvWgttZBIP3eEnMuGDZdihc2dVFsN 5UsMPN9JDV+SYkeJQKuMvdBaSzilehL9HlZpqg2owCsoxOjV6rFUHzZbHyjj9uClCY7+em RD73hKLXVV7ldsWilEwMNyHdCCU3MckK20504N3nDeDMdfrEaYqMF8rchRAXCS0abss2rT WhdTpkvlhxeZurUXdQ08tfwcROlqzr0HqdFBWDaBqPhMzPEeCO6e0QiqlCFYnO75txcA2s d/SFAKMeu8Dpovwn07sshv7LpSFfZ/SyJZISOeRSr6JWM707VibYcaVlEPQTSydl3xLKGY L5JKz/63Ra14ouspjlg7Lt+PBcbBpx2aF8ZrwXsrNRpq9K0Vr8MSDm8BZ5Dg X-ME-Proxy: Feedback-ID: i3a164872:Fastmail Received: by mailuser.phl.internal (Postfix, from userid 501) id D23D9700065; Sat, 22 Aug 2026 15:24:16 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Date: Sat, 22 Aug 2026 14:23:56 -0500 From: "John Groves" To: "Darrick J . Wong" Cc: "Richard Cheng" , "John Groves" , "Miklos Szeredi" , "Dan Williams" , "Bernd Schubert" , "Alison Schofield" , "John Groves (jgroves)" , "Jonathan Corbet" , "Jake Edge" , "Shuah Khan" , "Vishal Verma" , "Dave Jiang" , "Matthew Wilcox" , "Jan Kara" , "Alexander Viro" , "David Hildenbrand" , "Christian Brauner" , "Randy Dunlap" , "Jeff Layton" , "Amir Goldstein" , "Jonathan Cameron" , "Stefan Hajnoczi" , "Joanne Koong" , "Josef Bacik" , "Bagas Sanjaya" , "Chen Linxuan" , "James Morse" , "Fuad Tabba" , "Sean Christopherson" , "Shivank Garg" , "Ackerley Tng" , "Gregory Price" , "Andrew Morton" , "Namjae Jeon" , "Lorenzo Stoakes" , "Greg Kroah-Hartman" , "Ira Weiny" , "Pasha Tatashin" , "Haren Myneni" , "Pratyush Yadav" , "Giovanni Cabiddu" , "Jiri Slaby" , "Ethan Nelson-Moore" , "Gabriel Whigham" , "Aravind Ramesh" , "Ajay Joshi" , "venkataravis@micron.com" , "linux-doc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "nvdimm@lists.linux.dev" , "linux-cxl@vger.kernel.org" , "linux-fsdevel@vger.kernel.org" , "fuse-devel@lists.linux.dev" Message-Id: <95c95a40-fd7f-4a4e-b257-818caddcb3f1@app.fastmail.com> In-Reply-To: <20260822001723.GC6047@frogsfrogsfrogs> References: <0100019fed5850ec-2bdfb17a-3086-44ea-8fdd-777d3ce12a33-000000@email.amazonses.com> <20260810202510.96332-1-john@jagalactic.com> <0100019fed59de28-dde6a382-affe-4bc6-a91d-d4054a7c61a5-000000@email.amazonses.com> <20260822001723.GC6047@frogsfrogsfrogs> Subject: Re: [PATCH v13 07/12] famfs: MAP_CREATE ioctl and fmap ingest (ABI 44) Content-Type: text/plain Content-Transfer-Encoding: 7bit X-Stat-Signature: 64en7acjof16s9wcfxefh8ihdtbguwyj X-Rspamd-Server: rspamout04 X-Rspamd-Queue-Id: 9797B2002A X-Session-Marker: 6A6F686E4067726F7665732E6E6574 X-Session-ID: U2FsdGVkX1+3pFRhriOVOENpbcgb600B3/Cigfvb/HE= X-HE-Tag: 1787426657-749633 X-HE-Meta: U2FsdGVkX19jEgntu6Cf53RaD2DMvrvqkdr0cAfO6Aukd3vn2tbRSg4PmH2A562kSjv4crXfh4f6puI1iJ/a7P19UCV/UNgu+sXWPLH4nR2leb/R4Sjks8SbDOaCjHepL7C7ZK3d+JiQnQ0UN3Bs3qoCvyfmTOCeoPfayQLNn8CkNsFNeMgPOeYzyy2bLWUSQyWBxpaU7HrHPOLe1b/GQUcFugDKYX1jywNsvtCGPfV9Zx/MgimjQ6CnmofWUtW85f1TR+p3fSI127LpikPKEQvGadJQJjL0KelagNasnNsbhGdVAzFeGLZ/nIQIN5jL On Fri, Aug 21, 2026, at 7:17 PM, Darrick J. Wong wrote: > On Tue, Aug 11, 2026 at 05:17:50PM -0500, John Groves wrote: > > > > > > > + switch (fmh.ext_type) { > > > > + case FAMFS_IOC_EXT_SIMPLE: { > > > > + struct famfs_ioc_simple_ext *se_in = fmap_buf + next_offset; > > > > + > > > > + next_offset += (size_t)fmh.nextents * sizeof(*se_in); > > > > + if (next_offset > fmh.fmap_size) { > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + > > > > + meta->fm_nextents = fmh.nextents; > > > > + meta->se = kcalloc(meta->fm_nextents, sizeof(*meta->se), > > > > + GFP_KERNEL); > > > > + if (!meta->se) { > > > > + rc = -ENOMEM; > > > > + goto out; > > > > + } > > > > + > > > > + for (i = 0; i < fmh.nextents; i++) { > > > > + meta->se[i].dev_index = se_in[i].se_devindex; > > > > + meta->se[i].ext_offset = se_in[i].se_offset; > > > > + meta->se[i].ext_len = se_in[i].se_len; > > > > + > > > > + if (meta->se[i].dev_index >= FAMFS_MAX_DAXDEVS) { > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + meta->dev_bitmap |= BIT_ULL(meta->se[i].dev_index); > > > > + errs += famfs_check_ext_alignment(&meta->se[i]); > > > > + extent_total += meta->se[i].ext_len; > > > > > > offset + length + entent_total can overflow, and file_size can be larger > > > than MAX_LFS_FILESIZE. And overflow can wrap the DAX address back to 0 > > > and map the wrong memory. > > > > > > Maybe check_add_overflow() can be utilized ? > > > > Good idea, thanks! > > Yes, all those arithmetics should catch overflows. > > > > > > > > > > > + } > > > > + break; > > > > + } > > > > + > > > > + case FAMFS_IOC_EXT_INTERLEAVE: { > > > > + s64 size_remainder = meta->file_size; > > > > + u32 niext = fmh.nextents; > > > > + > > > > + meta->fm_niext = niext; > > > > + meta->ie = kcalloc(niext, sizeof(*meta->ie), GFP_KERNEL); > > > > + if (!meta->ie) { > > > > + rc = -ENOMEM; > > > > + goto out; > > > > + } > > > > + > > > > + /* Outer loop is over the separate interleaved extents */ > > > > + for (i = 0; i < niext; i++) { > > > > + struct famfs_ioc_iext *ie_in = fmap_buf + next_offset; > > > > + struct famfs_ioc_simple_ext *sie_in; > > > > + u64 nstrips; > > > > + > > > > + next_offset += sizeof(*ie_in); > > > > + if (next_offset > fmh.fmap_size) { > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + > > > > + /* chunk_size must be exactly one supported alloc unit */ > > > > + if (ie_in->ie_chunk_size != PAGE_SIZE && > > > > + ie_in->ie_chunk_size != PMD_SIZE) { > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + if (ie_in->ie_nbytes == 0) { > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + > > > > + nstrips = ie_in->ie_nstrips; > > > > + if (nstrips < 1) { > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + > > > > + meta->ie[i].fie_chunk_size = ie_in->ie_chunk_size; > > > > + meta->ie[i].fie_nstrips = ie_in->ie_nstrips; > > > > + meta->ie[i].fie_nbytes = ie_in->ie_nbytes; > > > > + > > > > + /* The strip extents follow the interleaved-ext header */ > > > > + sie_in = fmap_buf + next_offset; > > > > + next_offset += nstrips * sizeof(*sie_in); > > > > + if (next_offset > fmh.fmap_size) { > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + > > > > + meta->ie[i].ie_strips = > > > > + kcalloc(nstrips, > > > > + sizeof(meta->ie[i].ie_strips[0]), > > > > + GFP_KERNEL); > > > > + if (!meta->ie[i].ie_strips) { > > > > + rc = -ENOMEM; > > > > + goto out; > > > > + } > > > > + > > > > + /* Inner loop is over the strips */ > > > > + for (j = 0; j < nstrips; j++) { > > > > + struct famfs_meta_simple_ext *so = > > > > + &meta->ie[i].ie_strips[j]; > > > > + > > > > + so->dev_index = sie_in[j].se_devindex; > > > > + so->ext_offset = sie_in[j].se_offset; > > > > + so->ext_len = sie_in[j].se_len; > > > > + > > > > + if (so->dev_index >= FAMFS_MAX_DAXDEVS) { > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + meta->dev_bitmap |= BIT_ULL(so->dev_index); > > > > + errs += famfs_check_ext_alignment(so); > > > > + extent_total += so->ext_len; > > > > + size_remainder -= so->ext_len; > > > > > > We use physical allocation size here to check logical file coverage, > > > it doesn't make sense to me. > > > For example, file_size can be 1MB and ie_nbytes only 4KB, but a 1MB ext_len makes this check pass, and you access the area after the first 4 KB will fail, > > > because it has no logical mapping. > > > > > > Maybe we should make sure the sum of ie_nbytes covers file_size, and > > > separately check that each strip's ext_len is large enough for its assigned > > > chunks ? > > > > Chapeau to you for actually studying this code. Not many have gone > > there. It's arcane, but logically not too complicated. > > > > You're right that famfs_file_init_dax() doesn't check for pathological > > strip sizes or overflows. It does do some basic checking, but would > > not catch a short strip followed by one or more "correct" strips. > > That stuff would indicate a buggy or malicious caller, since it > > violates the fmap logic. > > > > However, the vma fault handler for interleaved files, > > famfs_meta_to_dax_offset_interleaved(), *does* check for strip > > overflows. It resolves a file offset to an offset in a specific > > strip (based on strip count and chunk size), and then checks that > > it falls within the strip and not past the end. > > > > Here is that code: > > > > /* > > * MAP_CREATE only checks that the strips' combined > > * length covers the file, not that each strip is large > > * enough for the chunks striped onto it. Guard against a > > * malformed fmap with an undersized strip so we never > > * resolve to a dax offset past the strip's extent. > > */ > > if (strip_offset >= strip->ext_len) > > goto err_out; > > > > daxdev = famfs_daxdev_from_index(fsi, strip->dev_index, &rc); > > if (!daxdev) { > > meta->error = true; > > return rc; > > } > > > > iomap->addr = strip->ext_offset + strip_offset; > > iomap->offset = file_offset; > > iomap->length = min_t(loff_t, len, chunk_remainder); > > iomap->length = min_t(loff_t, iomap->length, > > strip->ext_len - strip_offset); > > iomap->dax_dev = daxdev; > > iomap->type = IOMAP_MAPPED; > > > > return 0; > > > > Since this condition is an error or malice on the part of the > > MAP_CREATE caller, I'm comfortable with catching it at fault time. > > I think you should reject *any* bad mapping data at MAP_CREATE time > because then you can catch application bugs early with an immediate > error being sent to the famfs server. Don't let bad data into the > kernel. I can do that. I will note that the only application that does this (file creation during creat, cp or logplay in the famfs cli) definitively never has a shorter strip extent followed by a longer one (which would be true for there to be a strip overflow that wasn't caught till fault() time. but it's easy enough to check at MAP_CREATE so will do. > > > > > + } > > > > + } > > > > + > > > > + if (size_remainder > 0) { > > > > + /* Strips do not cover the whole file */ > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + break; > > > > + } > > > > + > > > > + default: > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + > > > > + if (errs > 0) { > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + if (extent_total < meta->file_size) { > > > > + rc = -EINVAL; > > > > + goto out; > > > > + } > > > > + > > > > + /* Publish the famfs metadata on inode->i_private */ > > > > + inode_lock(inode); > > > > + if (inode->i_private) { > > > > + rc = -EEXIST; /* file already has famfs metadata */ > > > > + } else { > > > > + inode->i_private = meta; > > > > + i_size_write(inode, meta->file_size); > > > > + inode->i_flags |= S_DAX; > > > > + meta = NULL; /* owned by the inode now */ > > > > + rc = 0; > > > > + } > > > > + inode_unlock(inode); > > > > + > > > > +out: > > > > + kvfree(fmap_buf); > > > > + if (meta) > > > > + famfs_meta_free(meta); > > > > + return rc; > > > > +} > > > > + > > > > +/** > > > > + * famfs_file_ioctl() - Top-level famfs file ioctl handler > > > > + * @file: the file > > > > + * @cmd: ioctl opcode > > > > + * @arg: ioctl opcode argument (if any) > > > > + */ > > > > +static long > > > > +famfs_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > > > > +{ > > > > + struct inode *inode = file_inode(file); > > > > + struct famfs_fs_info *fsi = inode->i_sb->s_fs_info; > > > > + long rc; > > > > + > > > > + if (fsi->deverror && (cmd != FAMFSIOC_NOP)) > > > > + return -ENODEV; > > > > + > > > > + switch (cmd) { > > > > + case FAMFSIOC_NOP: > > > > + rc = 0; > > > > + break; > > > > + > > > > + case FAMFSIOC_MAP_CREATE: > > > > + rc = famfs_file_init_dax(file, (void __user *)arg); > > > > + break; > > > > + > > > > + default: > > > > + rc = -ENOTTY; > > > > + break; > > > > + } > > > > + > > > > + return rc; > > > > +} > > > > + > > > > /********************************************************************* > > > > * vm_operations > > > > */ > > > > @@ -94,9 +400,25 @@ const struct vm_operations_struct famfs_file_vm_ops = { > > > > static ssize_t > > > > famfs_file_invalid(struct inode *inode) > > > > { > > > > + struct famfs_file_meta *meta = inode->i_private; > > > > + size_t i_size = i_size_read(inode); > > > > + > > > > + if (!meta) { > > > > + pr_debug("%s: un-initialized famfs file\n", __func__); > > > > + return -EIO; > > > > + } > > > > + if (meta->error) { > > > > + pr_debug("%s: previously detected metadata errors\n", __func__); > > > > + return -EIO; > > > > + } > > > > + if (i_size != meta->file_size) { > > > > + pr_warn("%s: i_size overwritten from %ld to %ld\n", > > > > + __func__, meta->file_size, i_size); > > > > + meta->error = true; > > > > + return -ENXIO; > > > > + } > > > > if (!IS_DAX(inode)) { > > > > - pr_debug("%s: inode %llx IS_DAX is false\n", > > > > - __func__, (u64)inode); > > > > + pr_debug("%s: inode %llx IS_DAX is false\n", __func__, (u64)inode); > > > > return -ENXIO; > > > > } > > > > return 0; > > > > @@ -233,7 +555,7 @@ const struct file_operations famfs_file_operations = { > > > > /* Custom famfs operations */ > > > > .write_iter = famfs_dax_write_iter, > > > > .read_iter = famfs_dax_read_iter, > > > > - .unlocked_ioctl = NULL /*famfs_file_ioctl*/, > > > > + .unlocked_ioctl = famfs_file_ioctl, > > > > .mmap = famfs_file_mmap, > > > > > > > > > > We don't have compat_ioctl handler, a 32-bit application on a 64-bit > > > kernel will get ENOTTY, if we will have that scenario I think the handler > > > should be added. > > > > > > Best regards, > > > Richard Cheng. > > > > This one is simple but arcane. Here are the kconfig deps: > > > > FAMFS -> FS_DAX -> ZONE_DEVICE -> MEMORY_HOTPLUG > > > > And MEMORY_HOTPLUG is depends on 64BIT. So famfs is definitively 64BIT-only. > > > > In this series I added a direct dependency on 64BIT, to make it more > > clear... > > compat_ioctl is for 32-bit programs calling into a 64-bit kernel. > > Granted a 32-bit program is probably not a good contender for famfs due > to limited address space, and you could simply declare that you don't > support 32-bit programs. > > --D > V13 already depends on 64BIT, so I think this is already done. 32 bit address space is not big enough to cover any of the intended use cases for famfs. Thanks Darrick! John