[prev in list] [next in list] [prev in thread] [next in thread]
List: autofs
Subject: Re: [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value
From: Tomohiro Kusumi <kusumi.tomohiro () gmail ! com>
Date: 2016-07-21 18:12:50
Message-ID: CAHndrBmA97DayAxH8RLzL4qGz0aktWCLTtxHJ32medYsPgSwgg () mail ! gmail ! com
[Download RAW message or body]
Thanks for another detailed explanation.
> And generally all patches that are committed and pushed are also available on
> kernel.org grouped by release. I've been trying my best to ensure they is as
> accurate as possible so it is clear what has gone into each release.
I'm assuming you're talking about
https://www.kernel.org/pub/linux/daemons/autofs/
for userspace stuff.
> Also, at times I have tried to put together a TODO list of longer term things
> that I'd like to do for myself but haven't been very successful, ;)
It would be cool to know the long term goal, if you can (or want to)
make it public.
2016-07-21 11:02 GMT+09:00 Ian Kent <raven@xxxxxxxxxx>:
> On Thu, 2016-07-21 at 02:53 +0900, Tomohiro Kusumi wrote:
> > > So I think the problem with the miscellaneous device ioctl number check
> > > you've
> > > pointed out does need to be fixed and if you could update the patch to do
> > > that
> > > it would be much appreciated.
> >
> > OK, I'll take a look at it again based on your explanation,
> > and see if I can fix it, plus make things a bit more clear if possible.
> >
> > > Don't forget that the miscellaneous device ioctls are a newer set of ioctls
> > > with
> > > added functionality to the older ioctls, hence the duplication you see with
> > > the
> > > functions provided with each set, and the check for that specific range is
> > > relevant to the later ones only.
> > >
> > > Also, only one or the other set of ioctls can be used and the miscellaneous
> > > device ioctls can only be used with version 5 and above of user space
> > > autofs.
> >
> > Yes, I was wondering why they're implemented to both fs root and /dev/autofs
> > when I first looked at ioctl and userspace code.
> >
> > >
> > > I'd like to try (again) to re-implement this with netlink and add readdir
> > > and
> > > asynchronous expire functionality but I don't know when I'll get time to do
> > > it s
> > > ince it is a lot of work in kernel and user space, but I digress.
> >
> > Is there a publicly available list of TODO for autofs (kernel and userspace) ?
> >
> > According to some of your recent posts to this mailing list,
> > you seem to have lots of patches that haven't been merged to upstream,
> > and also things yet to be implemented.
>
> There isn't.
>
> The truth is there haven't been other developers that have stuck with it so I've
> pretty much done everything in isolation.
>
> That's not too good because that means there isn't discussion about how things
> should be done and there's very little review of changes too, since there's no
> -one to review them.
>
> If there were others, such as yourself, I would start doing things quite
> differently, but clearly that would take time to start happening.
>
> I detest writing and maintaining web presences so there is no wiki which is
> another problem, and is the sort of place where a TODO list would reside.
>
> As far as patches that are either not finished, did not solve a problem, or are
> in progress, yes I have plenty of those and they have accumulated over many
> years.
>
> That's not uncommon when you work with something for a long time but they are not \
> useful until you return to a problem they may be related too, so mostly not useful \
> to others.
> For example, I tried to implement the control interface using the netlink
> mechanism, a long time ago now, and failed but I keep the work that I did in
> case I can use it at some time is the future, that's my nature.
>
> A netlink interface would be better than using ioctls, it would be more flexible
> in the long run but, at the time, I was unwilling to fundamentally change the
> way automount(8) works and there were other difficulties with asynchronous
> expires so I failed to get it working and implemented the anonymous device ioctl
> interface instead.
>
> So these patches are not useful at all and are just a consequence of things I
> have tried to do in the past.
>
> But there is still the question of how to better deal with listing of
> directories (using certain options) that can cause everything within the
> directory to mount.
>
> Over the years I've thought about it many times and at times I think a kernel
> readdir implementation would be better and (often after thinking more about it)
> I come back to there being no advantage over the existing pre-creation of mount
> point directories.
>
> If I was to, one day, believe that a kernel readdir implementation would make a
> worthwhile difference then I think it would be better to implement that using
> the netlink sub-system and the problems with that would need to be worked out to
> do so.
>
> So it ends up being little more than something for thought only, or possibly
> discussion if there were others to take part in it, and what I have for this
> would be extremely hard for anyone else to use as it is a mess of partially done
> work.
>
> For a long time now I accumulate patches and then commit and push those that
> prove sound. And often it has been quite a while between pushes so there can end
> up being quite a few, but that can change if there is reason to do so.
>
> And generally all patches that are committed and pushed are also available on
> kernel.org grouped by release. I've been trying my best to ensure they is as
> accurate as possible so it is clear what has gone into each release.
>
> Also, at times I have tried to put together a TODO list of longer term things
> that I'd like to do for myself but haven't been very successful, ;)
>
> >
> >
> > 2016-07-20 9:47 GMT+09:00 Ian Kent <raven@xxxxxxxxxx>:
> > > On Wed, 2016-07-20 at 04:29 +0900, Tomohiro Kusumi wrote:
> > > > Thanks for your review,
> > > >
> > > > Yes, yours apparently works and that's exactly the right test based on
> > > > currently existing ioctl cmds.
> > > > I first thought about changing it like you did, but thought about
> > > > meaning of AUTOFS_IOC_COUNT which is 32.
> > >
> > > I think the meaning of this is taken from Documentation/ioctl/ioctl
> > > -number.txt.
> > >
> > > The define isn't actually used though, which is what I was saying.
> > >
> > > >
> > > > I took this 32 as some sort of reserved range for autofs ioctl cmds,
> > > > where the whole ioctl cmds are designed to be somewhere in 0x60-0x80
> > > > range, and /dev/autofs ioctl cmds are in 0x71-0x80 range.
> > >
> > > Yes, if ioctl numbers beyond 0x7e are to be used I think we need to update
> > > the
> > > above file.
> > >
> > > It's been a while since I did this but the highest miscellaneous device
> > > ioctl
> > > number is 0x7e so if new ioctls were needed the usage registration file
> > > would
> > > need to be updated and posted as a patch.
> > >
> > > > So in theory a potential ioctl cmd for /dev/autofs counting from the
> > > > smallest cmd must be no larger than 32-0x11 (or 0x80-0x71).
> > > >
> > > > I've no idea what this reservation/limitation up to 0x80 is supposed
> > > > to mean though, as ioctl has 8 bits for nr part.
> > > > If you say this 32 is meaningless, I'll resubmit it with your proposed
> > > > change.
> > >
> > >
> > > The AUTOFS_IOC_COUNT should stay purely because it documents the "claimed"
> > > autofs range (from 0x60-0x7f). Maybe a comment is needed to clear this up.
> > >
> > > But AUTOFS_DEV_IOCTL_IOC_COUNT is supposed to be used to check the passed in
> > > miscellaneous device ioctl number is between 0x71-0x7f (or would if it was
> > > correct).
> > >
> > > So I think the problem with the miscellaneous device ioctl number check
> > > you've
> > > pointed out does need to be fixed and if you could update the patch to do
> > > that
> > > it would be much appreciated.
> > >
> > > Don't forget that the miscellaneous device ioctls are a newer set of ioctls
> > > with
> > > added functionality to the older ioctls, hence the duplication you see with
> > > the
> > > functions provided with each set, and the check for that specific range is
> > > relevant to the later ones only.
> > >
> > > Also, only one or the other set of ioctls can be used and the miscellaneous
> > > device ioctls can only be used with version 5 and above of user space
> > > autofs.
> > >
> > > I'd like to try (again) to re-implement this with netlink and add readdir
> > > and
> > > asynchronous expire functionality but I don't know when I'll get time to do
> > > it s
> > > ince it is a lot of work in kernel and user space, but I digress.
> > >
> > > >
> > > >
> > > > 0x60 0x71 0x80
> > > > -----+------------------+---------------+------------------> NR
> > > > <----------------------------------> AUTOFS_IOC_COUNT (32)
> > > > <---------------> AUTOFS_DEV_IOCTL_IOC_COUNT
> > >
> > > > 2016-07-19 10:34 GMT+09:00 Ian Kent <raven@xxxxxxxxxx>:
> > > > > On Sun, 2016-07-10 at 17:07 +0900, Tomohiro Kusumi wrote:
> > > > > > According to include/uapi/linux/auto_fs.h,
> > > > > > include/uapi/linux/auto_fs4.h,
> > > > > > and include/linux/auto_dev-ioctl.h, nr part of
> > > > > > AUTOFS_IOC_XXX starts off from 0x60 and
> > > > > > AUTOFS_DEV_IOCTL_XXX starts off from 0x71 which is 0x60+0x11.
> > > > > >
> > > > > > From the way above macros are defined, it seems
> > > > > > AUTOFS_DEV_IOCTL_IOC_COUNT should be (0x20-0x11) instead of (0x20-11).
> > > > > > (Otherwise it's hard to figure out where 11 comes from)
> > > > >
> > > > > You're correct, this is wrong but I think this change is also wrong.
> > > > >
> > > > > And so is the range check in dev-ioctl.c.
> > > > >
> > > > > There's no check for an ioctl number greater than the range claimed by
> > > > > autofs
> > > > > and the check in dev-ioctl.c seeks to check the number used is within
> > > > > the
> > > > > range
> > > > > used by the dev ioctls.
> > > > >
> > > > > Essentially
> > > > >
> > > > > AUTOFS_DEV_IOCTL_VERSION_CMD - AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD = 0xd
> > > > >
> > > > > 14 ioctls ranging from 0x71 - 0x7e (inclusive).
> > > > >
> > > > > The range check in dev-ioctl.c is:
> > > > >
> > > > > if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST)
> > > > > > >
> > > > > cmd - cmd_first >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
> > > > > return -ENOTTY;
> > > > > }
> > > > >
> > > > > so I think AUTOFS_DEV_IOCTL_IOC_COUNT should be:
> > > > >
> > > > > #define AUTOFS_DEV_IOCTL_IOC_COUNT \
> > > > > (AUTOFS_DEV_IOCTL_VERSION_CMD -
> > > > > AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD)
> > > > >
> > > > > and the check should be:
> > > > >
> > > > > if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST)
> > > > > > >
> > > > >
> > > > > cmd_first - cmd >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
> > > > > return
> > > > > -ENOTTY;
> > > > > }
> > > > >
> > > > > What do you think?
> > > > >
> > > > > >
> > > > > > COUNT macros are being used to test distance of an ioctl command
> > > > > > in question from the smallest one, so we don't want it to be
> > > > > > larger than necessary.
> > > > > >
> > > > > > Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@xxxxxxxxx>
> > > > > > ---
> > > > > > fs/autofs4/autofs_i.h | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > > > > > index 73e3d38..7ef4d08 100644
> > > > > > --- a/fs/autofs4/autofs_i.h
> > > > > > +++ b/fs/autofs4/autofs_i.h
> > > > > > @@ -20,7 +20,7 @@
> > > > > > #define AUTOFS_IOC_COUNT 32
> > > > > >
> > > > > > #define AUTOFS_DEV_IOCTL_IOC_FIRST (AUTOFS_DEV_IOCTL_VERSION)
> > > > > > -#define AUTOFS_DEV_IOCTL_IOC_COUNT (AUTOFS_IOC_COUNT - 11)
> > > > > > +#define AUTOFS_DEV_IOCTL_IOC_COUNT (AUTOFS_IOC_COUNT - 0x11)
> > > > > >
> > > > > > #include <linux/kernel.h>
> > > > > > #include <linux/slab.h>
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic