| Message ID | 685c8abce7bdb110bc306752314b4fb0e7867290.1662420176.git.sweettea-kernel@dorminy.me (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | btrfs: add fscrypt integration | expand |
On Mon, Sep 05, 2022 at 08:35:25PM -0400, Sweet Tea Dorminy wrote: > From: Omar Sandoval <osandov@osandov.com> > > Now that everything in btrfs is dealing in fscrypt_names, fscrypt has a > useful function, fscrypt_match_name(), to check whether a fscrypt_name > matches a provided buffer. However, btrfs buffers are struct > extent_buffer rather than a raw char array, so we need to implement our > own imitation of fscrypt_match_name() that deals in extent_buffers, > falling back to a simple memcpy if fscrypt isn't compiled. We > can then use this matching method in btrfs_match_dir_item_name() and > other locations. > > This also provides a useful occasion to introduce the new fscrypt file > for btrfs, handling the fscrypt-specific functions needed. > > Signed-off-by: Omar Sandoval <osandov@osandov.com> > Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> The code is fine, but I was very confused about why we do this sha256 thing. Perhaps point at the code for fscrypt_nokey_name and indicate that it exists to be interoperable with no-key actions on the file system. Thanks, Josef
On Mon, Sep 05, 2022 at 08:35:25PM -0400, Sweet Tea Dorminy wrote: > From: Omar Sandoval <osandov@osandov.com> > @@ -0,0 +1,32 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2020 Facebook > + */ Please use only SPDX in new files https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#Copyright_notices_in_files.2C_SPDX
On Fri, Sep 09, 2022 at 12:15:21PM +0200, David Sterba wrote: > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2020 Facebook > > + */ > > Please use only SPDX in new files > > https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#Copyright_notices_in_files.2C_SPDX The wiki is incorrect. The SPDX tag deals with the licensing tags only. It is not a replacement for the copyright notice in any way, and having been involved with Copyright enforcement I can tell you that at least in some jurisdictions Copytight notices absolutely do matter.
On Fri, Sep 09, 2022 at 06:00:13AM -0700, Christoph Hellwig wrote: > On Fri, Sep 09, 2022 at 12:15:21PM +0200, David Sterba wrote: > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright (C) 2020 Facebook > > > + */ > > > > Please use only SPDX in new files > > > > https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#Copyright_notices_in_files.2C_SPDX > > The wiki is incorrect. The SPDX tag deals with the licensing tags > only. It is not a replacement for the copyright notice in any way, and > having been involved with Copyright enforcement I can tell you that > at least in some jurisdictions Copytight notices absolutely do matter. I believe you and can update the wiki text so it's more explicit about the license an copyright. Otherwise I don't see much point in the copyright notices unless they're complete list of every person and company that touched that file. With that we haven't been adding that to new files for some time and I want to be consistent with that. In each case the patch author was asked to resubmit without the notice. It worked so far. If not, either the patch will be respectfully rejected or somebody else creates the file first and then the patch applied.
On 9/9/22 9:00 AM, Christoph Hellwig wrote: > On Fri, Sep 09, 2022 at 12:15:21PM +0200, David Sterba wrote: >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Copyright (C) 2020 Facebook >>> + */ >> >> Please use only SPDX in new files >> >> https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#Copyright_notices_in_files.2C_SPDX > > The wiki is incorrect. The SPDX tag deals with the licensing tags > only. It is not a replacement for the copyright notice in any way, and > having been involved with Copyright enforcement I can tell you that > at least in some jurisdictions Copytight notices absolutely do matter. I agree, SPDX != copyright, and they should probably be in different paragraphs in the wiki because it's misleading as it stands. Signed-off-by is targeted at declaring that you have permission to put this code into this project, which is somewhat different from copyright. For Meta code, I'm happy to use the git history method. But, I'm not completely comfortable with enforcing that on other companies or individuals unless it's common practice elsewhere in the kernel. -chris
On 9/9/22 7:34 AM, David Sterba wrote: > On Fri, Sep 09, 2022 at 06:00:13AM -0700, Christoph Hellwig wrote: >> On Fri, Sep 09, 2022 at 12:15:21PM +0200, David Sterba wrote: >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * Copyright (C) 2020 Facebook >>>> + */ >>> Please use only SPDX in new files >>> >>> https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#Copyright_notices_in_files.2C_SPDX >> The wiki is incorrect. The SPDX tag deals with the licensing tags >> only. It is not a replacement for the copyright notice in any way, and >> having been involved with Copyright enforcement I can tell you that >> at least in some jurisdictions Copytight notices absolutely do matter. > I believe you and can update the wiki text so it's more explicit about > the license an copyright. Can you update the wiki text to remove "SPDX" from the heading and remove the sentence stating, "An initiative started in 2017 [1] aims to unify licensing information in all files using SPDX tags, this is driven by the Linux Foundation." Thanks, Jilayne
Regarding https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#Copyright_notices_in_files.2C_SPDX On Fri, Sep 09, 2022 at 06:00:13AM -0700, Christoph Hellwig wrote: > > > The wiki is incorrect. The SPDX tag deals with the licensing tags > > > only. It is not a replacement for the copyright notice in any way, and > > > having been involved with Copyright enforcement I can tell you that at > > > least in some jurisdictions Copytight notices absolutely do matter. This is a very good point. The current Wiki page for btrfs (linked above) says: > There's no need to put the copyright notices in individual files that are > new, renamed or split. … > Note that removing the copyright from existing files is not trivial and > would require asking the original authors or current copyright holders. The > status will be inconsistent but at least new contributions won't continue > adding new ones. The current licensing practices are believed to be > sufficient. This is admittedly a very tough problem to solve. Nevertheless, the concern that I have with that recommendation above is that it gives copyright holders whose notices are grandfathered an additional notice preservation that new copyright holders don't have equal access to. It's particular problematic because new contributors are unable to have contributions included unless they remove copyright notices. Again, I realize the trade-offs are really tough here; removing existing copyright notices without explicit permission is a *serious* problem (both a GPL violation and a statutory violation of copyright generally in many jurisdictions). OTOH, a list of every last copyright holder is painfully unwieldy — even if you combine it into a single location. Most importantly, I want to point out the bigger, implicit trade-off here that some may not realize. If you relying on Git history to have copyright notice information, it does make the entire Git repository a required part of the complete, corresponding source under GPLv2. This will become even more certain when contributors are being told that they may *not* include a copyright notice and that their copyright information will appear in metadata instead. They can reasonably interpret the “appropriately publish on each copy an appropriate copyright notice” in GPLv2§1 to mean the copyright notices in the Git metadata. J Lovejoy wrote: > Can you update the wiki text to remove "SPDX" from the heading and remove > the sentence stating, "An initiative started in 2017 [1] aims to unify > licensing information in all files using SPDX tags, this is driven by the > Linux Foundation." All of that seems accurate to me. What part is not accurate? Splitting the information to talk about copyright and license separately seems a good idea, but removing accurate explanations doesn't seem like a good idea to me … -- bkuhn
On Fri, Sep 16, 2022 at 04:18:47PM -0600, J Lovejoy wrote: > > > On 9/9/22 7:34 AM, David Sterba wrote: > > On Fri, Sep 09, 2022 at 06:00:13AM -0700, Christoph Hellwig wrote: > >> On Fri, Sep 09, 2022 at 12:15:21PM +0200, David Sterba wrote: > >>>> +// SPDX-License-Identifier: GPL-2.0 > >>>> +/* > >>>> + * Copyright (C) 2020 Facebook > >>>> + */ > >>> Please use only SPDX in new files > >>> > >>> https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#Copyright_notices_in_files.2C_SPDX > >> The wiki is incorrect. The SPDX tag deals with the licensing tags > >> only. It is not a replacement for the copyright notice in any way, and > >> having been involved with Copyright enforcement I can tell you that > >> at least in some jurisdictions Copytight notices absolutely do matter. > > I believe you and can update the wiki text so it's more explicit about > > the license an copyright. > > Can you update the wiki text to remove "SPDX" from the heading and > remove the sentence stating, "An initiative started in 2017 [1] aims to > unify licensing information in all files using SPDX tags, this is driven > by the Linux Foundation." I can consider that if you tell me why I should do that, but I don't find anything wrong with the sentence (and I wrote it originally). It's merely stating that something happened and points to a well known linux kernel related web site with more details about it.
On Sun, Sep 18, 2022 at 07:00:17PM -0700, Bradley M. Kuhn wrote: > Regarding > https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#Copyright_notices_in_files.2C_SPDX > > On Fri, Sep 09, 2022 at 06:00:13AM -0700, Christoph Hellwig wrote: > > > > > The wiki is incorrect. The SPDX tag deals with the licensing tags > > > > only. It is not a replacement for the copyright notice in any way, and > > > > having been involved with Copyright enforcement I can tell you that at > > > > least in some jurisdictions Copytight notices absolutely do matter. > > This is a very good point. I've expanded the page hopefully correcting the confusion. It has 3 sections, about spdx, about copyright and the community perspective. > The current Wiki page for btrfs (linked above) says: > > There's no need to put the copyright notices in individual files that are > > new, renamed or split. > … > > Note that removing the copyright from existing files is not trivial and > > would require asking the original authors or current copyright holders. The > > status will be inconsistent but at least new contributions won't continue > > adding new ones. The current licensing practices are believed to be > > sufficient. > > This is admittedly a very tough problem to solve. Nevertheless, the concern > that I have with that recommendation above is that it gives copyright holders > whose notices are grandfathered an additional notice preservation that new > copyright holders don't have equal access to. It's particular problematic > because new contributors are unable to have contributions included unless > they remove copyright notices. > > Again, I realize the trade-offs are really tough here; removing existing > copyright notices without explicit permission is a *serious* problem (both a > GPL violation and a statutory violation of copyright generally in many > jurisdictions). OTOH, a list of every last copyright holder is painfully > unwieldy — even if you combine it into a single location. > > Most importantly, I want to point out the bigger, implicit trade-off here > that some may not realize. If you relying on Git history to have copyright > notice information, it does make the entire Git repository a required part of > the complete, corresponding source under GPLv2. This will become even more > certain when contributors are being told that they may *not* include a > copyright notice and that their copyright information will appear in metadata > instead. They can reasonably interpret the “appropriately publish on each > copy an appropriate copyright notice” in GPLv2§1 to mean the copyright > notices in the Git metadata. Thanks for the reply. Oh well, so we basically don't have good options.
diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile index 99f9995670ea..b6444490cdbc 100644 --- a/fs/btrfs/Makefile +++ b/fs/btrfs/Makefile @@ -38,6 +38,7 @@ btrfs-$(CONFIG_BTRFS_FS_CHECK_INTEGRITY) += check-integrity.o btrfs-$(CONFIG_BTRFS_FS_REF_VERIFY) += ref-verify.o btrfs-$(CONFIG_BLK_DEV_ZONED) += zoned.o btrfs-$(CONFIG_FS_VERITY) += verity.o +btrfs-$(CONFIG_FS_ENCRYPTION) += fscrypt.o btrfs-$(CONFIG_BTRFS_FS_RUN_SANITY_TESTS) += tests/free-space-tests.o \ tests/extent-buffer-tests.o tests/btrfs-tests.o \ diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c index b4c1e2a40401..8d7c3c32ed8e 100644 --- a/fs/btrfs/dir-item.c +++ b/fs/btrfs/dir-item.c @@ -5,6 +5,7 @@ #include "ctree.h" #include "disk-io.h" +#include "fscrypt.h" #include "transaction.h" /* @@ -390,15 +391,15 @@ struct btrfs_dir_item *btrfs_match_dir_item_name(struct btrfs_fs_info *fs_info, total_len = btrfs_item_size(leaf, path->slots[0]); while (cur < total_len) { - unsigned long name_ptr = (unsigned long)(dir_item + 1); - this_len = sizeof(*dir_item) + - btrfs_dir_name_len(leaf, dir_item) + + int dir_name_len = btrfs_dir_name_len(leaf, dir_item); + this_len = sizeof(*dir_item) + dir_name_len + btrfs_dir_data_len(leaf, dir_item); - if (btrfs_dir_name_len(leaf, dir_item) == fname_len(fname) && - memcmp_extent_buffer(leaf, fname_name(fname), name_ptr, - fname_len(fname)) == 0) + if (btrfs_fscrypt_match_name(fname, leaf, + (unsigned long)(dir_item + 1), + dir_name_len)) { return dir_item; + } cur += this_len; dir_item = (struct btrfs_dir_item *)((char *)dir_item + diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 591c191a58bc..a467a7553bd9 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 +#include <crypto/sha2.h> #include <linux/bitops.h> #include <linux/slab.h> #include <linux/bio.h> @@ -6892,6 +6893,42 @@ void memzero_extent_buffer(const struct extent_buffer *eb, unsigned long start, } } +void extent_buffer_sha256(const struct extent_buffer *eb, unsigned long start, + unsigned long len, u8 *out) +{ + size_t cur; + size_t offset; + struct page *page; + char *kaddr; + unsigned long i = get_eb_page_index(start); + struct sha256_state sctx; + + if (check_eb_range(eb, start, len)) + return; + + offset = get_eb_offset_in_page(eb, start); + + /* + * TODO: This should maybe be using the crypto API, not the fallback, + * but fscrypt uses the fallback and this is only used in emulation of + * fscrypt's buffer sha256 method. + */ + sha256_init(&sctx); + while (len > 0) { + page = eb->pages[i]; + assert_eb_page_uptodate(eb, page); + + cur = min(len, PAGE_SIZE - offset); + kaddr = page_address(page); + sha256_update(&sctx, (u8 *)(kaddr + offset), cur); + + len -= cur; + offset = 0; + i++; + } + sha256_final(&sctx, out); +} + void copy_extent_buffer_full(const struct extent_buffer *dst, const struct extent_buffer *src) { diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h index 69a86ae6fd50..d6c1b91cd690 100644 --- a/fs/btrfs/extent_io.h +++ b/fs/btrfs/extent_io.h @@ -222,6 +222,8 @@ void memmove_extent_buffer(const struct extent_buffer *dst, unsigned long len); void memzero_extent_buffer(const struct extent_buffer *eb, unsigned long start, unsigned long len); +void extent_buffer_sha256(const struct extent_buffer *eb, unsigned long start, + unsigned long len, u8 *out); int extent_buffer_test_bit(const struct extent_buffer *eb, unsigned long start, unsigned long pos); void extent_buffer_bitmap_set(const struct extent_buffer *eb, unsigned long start, diff --git a/fs/btrfs/fscrypt.c b/fs/btrfs/fscrypt.c new file mode 100644 index 000000000000..2ed844dd61d0 --- /dev/null +++ b/fs/btrfs/fscrypt.c @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020 Facebook + */ + +#include "ctree.h" +#include "fscrypt.h" + +/* fscrypt_match_name() but for an extent_buffer. */ +bool btrfs_fscrypt_match_name(const struct fscrypt_name *fname, + struct extent_buffer *leaf, unsigned long de_name, + u32 de_name_len) +{ + const struct fscrypt_nokey_name *nokey_name = + (const void *)fname->crypto_buf.name; + u8 digest[SHA256_DIGEST_SIZE]; + + if (likely(fname->disk_name.name)) { + if (de_name_len != fname->disk_name.len) + return false; + return !memcmp_extent_buffer(leaf, fname->disk_name.name, + de_name, de_name_len); + } + if (de_name_len <= sizeof(nokey_name->bytes)) + return false; + if (memcmp_extent_buffer(leaf, nokey_name->bytes, de_name, + sizeof(nokey_name->bytes))) + return false; + extent_buffer_sha256(leaf, de_name + sizeof(nokey_name->bytes), + de_name_len - sizeof(nokey_name->bytes), digest); + return !memcmp(digest, nokey_name->sha256, sizeof(digest)); +} diff --git a/fs/btrfs/fscrypt.h b/fs/btrfs/fscrypt.h new file mode 100644 index 000000000000..7f24d12e6ee0 --- /dev/null +++ b/fs/btrfs/fscrypt.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef BTRFS_FSCRYPT_H +#define BTRFS_FSCRYPT_H + +#include <linux/fscrypt.h> + +#ifdef CONFIG_FS_ENCRYPTION +bool btrfs_fscrypt_match_name(const struct fscrypt_name *fname, + struct extent_buffer *leaf, + unsigned long de_name, u32 de_name_len); + +#else +static bool btrfs_fscrypt_match_name(const struct fscrypt_name *fname, + struct extent_buffer *leaf, + unsigned long de_name, u32 de_name_len) +{ + if (de_name_len != fname->disk_name.len) + return false; + return !memcmp_extent_buffer(leaf, fname->disk_name.name, + de_name, de_name_len); +} +#endif + +#endif /* BTRFS_FSCRYPT_H */ diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c index 78053eb9589c..4ad75f9573aa 100644 --- a/fs/btrfs/inode-item.c +++ b/fs/btrfs/inode-item.c @@ -7,6 +7,7 @@ #include "ctree.h" #include "inode-item.h" #include "disk-io.h" +#include "fscrypt.h" #include "transaction.h" #include "print-tree.h" @@ -62,10 +63,9 @@ struct btrfs_inode_extref *btrfs_find_name_in_ext_backref( name_ptr = (unsigned long)(&extref->name); ref_name_len = btrfs_inode_extref_name_len(leaf, extref); - if (ref_name_len == fname_len(fname) && - btrfs_inode_extref_parent(leaf, extref) == ref_objectid && - (memcmp_extent_buffer(leaf, fname_name(fname), name_ptr, - fname_len(fname)) == 0)) + if (btrfs_inode_extref_parent(leaf, extref) == ref_objectid && + btrfs_fscrypt_match_name(fname, leaf, name_ptr, + ref_name_len)) return extref; cur_offset += ref_name_len + sizeof(*extref); diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 761293d763b6..482c5b3d9e70 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -55,6 +55,7 @@ #include "zoned.h" #include "subpage.h" #include "inode-item.h" +#include "fscrypt.h" struct btrfs_iget_args { u64 ino; @@ -5646,14 +5647,10 @@ static int fixup_tree_root_location(struct btrfs_fs_info *fs_info, leaf = path->nodes[0]; ref = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_root_ref); - if (btrfs_root_ref_dirid(leaf, ref) != btrfs_ino(BTRFS_I(dir)) || - btrfs_root_ref_name_len(leaf, ref) != dentry->d_name.len) + if (btrfs_root_ref_dirid(leaf, ref) != btrfs_ino(BTRFS_I(dir))) goto out; - - ret = memcmp_extent_buffer(leaf, dentry->d_name.name, - (unsigned long)(ref + 1), - dentry->d_name.len); - if (ret) + if (!btrfs_fscrypt_match_name(&fname, leaf, (unsigned long)(ref + 1), + btrfs_root_ref_name_len(leaf, ref))) goto out; btrfs_release_path(path); diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c index f44f02c22027..bbb215007896 100644 --- a/fs/btrfs/root-tree.c +++ b/fs/btrfs/root-tree.c @@ -8,6 +8,7 @@ #include "ctree.h" #include "transaction.h" #include "disk-io.h" +#include "fscrypt.h" #include "print-tree.h" #include "qgroup.h" #include "space-info.h" @@ -351,14 +352,14 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, if (ret < 0) { goto out; } else if (ret == 0) { + u32 name_len; leaf = path->nodes[0]; ref = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_root_ref); ptr = (unsigned long)(ref + 1); + name_len = btrfs_root_ref_name_len(leaf, ref); if ((btrfs_root_ref_dirid(leaf, ref) != dirid) || - (btrfs_root_ref_name_len(leaf, ref) != fname_len(fname)) || - memcmp_extent_buffer(leaf, fname_name(fname), ptr, - fname_len(fname))) { + !btrfs_fscrypt_match_name(fname, leaf, ptr, name_len)) { ret = -ENOENT; goto out; }