On Tue, Jul 12, 2022 at 7:58 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxx> wrote:
>
> Well, if you're supposed to be able to dedupe read-only files, then
> the whole "check for writing" is just bogus to begin with.
Hmm. Maybe that's too strong. The "if you can write to it, you can
always dedup it" does make sense, and then couple that with "other
situations are also ok".
And if you were to want to dedup things like symlinks (do people?) you
need to be able to handle FMODE_PATH cases that aren't FMODE_WRITE.
And at that point, the "inode owner" check starts making sense.
Except the code doesn't allow symlinks anyway right now, so that's
kind of theoretical.
But then you really do need to check for IS_IMMUTABLE there too and
that it's a valid file type, and not just hope that it's checked for
elsewhere. And those checks shouldn't pass just because of
CAP_SYS_ADMIN, so that check seems to be in the wrong place too.
So maybe it mostly works (apart from how clearly the destination
offset alignment check is somehow missing or done too late or
whatever), but it all seems very random, with checks spread out and
not with any consistency or logic.
As an example of this randomness, for the dedup source file, we have
three different error returns for checking whether it's ok in
vfs_dedupe_file_range(*):
if (S_ISDIR(src->i_mode))
return -EISDIR;
if (!S_ISREG(src->i_mode))
return -EINVAL;
if (!file->f_op->remap_file_range)
return -EOPNOTSUPP;
but then for the destination check the code does
ret = -EISDIR;
if (S_ISDIR(file_inode(dst_file)->i_mode))
goto out_drop_write;
ret = -EINVAL;
if (!dst_file->f_op->remap_file_range)
goto out_drop_write;
and again - maybe this works, and it's just that the error return is
inconsistent for source-vs-target, but it just reinforces that whole
"this is all completely ad-hoc and doesn't really make much sense".
Linus
