On Thu, Nov 02, 2017 at 12:50:07AM +0100, Luis R. Rodriguez wrote:
> On Tue, Oct 31, 2017 at 06:12:30PM -0500, Eric Sandeen wrote:
> >
> >
> > On 10/31/17 6:10 PM, Luis R. Rodriguez wrote:
> > > On Tue, Oct 31, 2017 at 03:43:20PM -0700, Darrick J. Wong wrote:
> > >> On Tue, Oct 31, 2017 at 03:19:00PM -0700, Luis R. Rodriguez wrote:
> > >>> On Tue, Oct 31, 2017 at 3:12 PM, Darrick J. Wong
> > >>> <darrick.wong@xxxxxxxxxx> wrote:
> > >>>> On Tue, Oct 31, 2017 at 02:51:56PM -0700, Luis R. Rodriguez wrote:
> > >>>>> diff --git a/repair/dinode.c b/repair/dinode.c
> > >>>>> index 15ba8cc22b39..6288e42de15e 100644
> > >>>>> --- a/repair/dinode.c
> > >>>>> +++ b/repair/dinode.c
> > >>>>> @@ -2482,6 +2482,27 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
> > >>>>> FS_XFLAG_EXTSIZE);
> > >>>>> }
> > >>>>> }
> > >>>>> + if (flags & (XFS_DIFLAG_IMMUTABLE | XFS_DIFLAG_APPEND |
> > >>>>> + XFS_DIFLAG_NODUMP)) {
> > >>>>> + /*
> > >>>>> + * ioctl(fd, *) and so ioctl(fd, FS_IOC_SETFLAGS)
> > >>>>> + * yields EBADF on symlinks as they have O_PATH set.
> > >>>>> + * "Extra file attributes", stx_attributes, as per
> > >>>>> + * statx(2) cannot be set on symlinks on Linux.
> > >>>>> + */
> > >>>>> + if (di_mode && S_ISLNK(di_mode) &&
> > >>>>> + !S_ISREG(di_mode) && !S_ISDIR(di_mode)) {
> > >>>>
> > >>>> Does this DIFLAG clearing applies to bdev/cdev/fifo/socket files too?
> > >>>
> > >>> Not at the moment given the semantics I hunted down and tested for
> > >>> were for O_PATH only. The validation I hunted down applies to any
> > >>> file descriptors which we open via O_PATH only.
> > >>
> > >> iirc when you open one of those special files you end up with a fd that
> > >> points to an inode on a special bdevfs/pipefs/etc., not an inode linked
> > >> to the underlying filesystem containing the special file.
> > >
> > > That seems to fit the O_PATH intent, however its unclear if O_PATH was needed,
> > > as per my testing on /dev/loop0 I don't need O_PATH set for it.
> > >
> > >> Therefore you shouldn't be able to set any DIFLAG/DIFLAG2 flags on special files.
> > >
> > > That would be great if we can verify.
> > >
> > >> # mknod block b 8 0 ; mknod char c 1 3 ; mknod fifo p
> > >> # lsattr block char fifo
> > >> lsattr: Operation not supported While reading flags on block
> > >> lsattr: Operation not supported While reading flags on char
> > >> lsattr: Operation not supported While reading flags on fifo
> > >
> > > I'm afraid e2fsprogs has a special check for these, ie, userspace is barred
> > > from actually toying with special files purposely because of the Debian bug I
> > > named.
> > >
> > > strace should reveal the respective ioctl() was not actually issued.
> >
> > That's very easy to short-circuit in e2fsprogs if you'd like to test what
> > the kernel does today.
>
> I meant that *I know* that the ioctl() is not issued, as it is a guard implemented
> on e2fsprogs to avoid potentially interfacing with buggy kernel drivers.
>
> One however can write a program which does the raw ioctl() call to really test.
>
> For all the above: block char fifo, ioctl() with FS_IOC_FSGETXATTR and
> FS_IOC_FSSETXATTR fails with: -1 an errno is set to ENOTTY (Inappropriate ioctl
> for device). Reason is that vfs_ioctl() is used, and on XFS we'll hit the goto
> out here as no f_op callbacks are set for these special devices:
>
> long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> int error = -ENOTTY;
>
> if (!filp->f_op->unlocked_ioctl)
> goto out;
>
> error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
> if (error == -ENOIOCTLCMD)
> error = -ENOTTY;
> out:
> return error;
> }
>
> My point also was that the above logic is different than for symlink. I'd much
> prefer to address these as a secondary patch given the logic is different.
>
> PF_LOCAL named sockets are also represented on the filesystem, and same situation
> with them, so I can bunch up a check for them as well. Are you OK with these
> going in as separate patches or do you want me to mesh this all together along
> with the new explanation for these?
*re-poke*
I hope the VFS RFC patch I just posted clarifies the situation a bit more.
Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
