From: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
To: Ian Kent <raven@themaw.net>
Cc: autofs@vger.kernel.org
Subject: Re: [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value
Date: Fri, 22 Jul 2016 03:12:08 +0900 [thread overview]
Message-ID: <CAHndrBmA97DayAxH8RLzL4qGz0aktWCLTtxHJ32medYsPgSwgg@mail.gmail.com> (raw)
In-Reply-To: <1469066569.18422.45.camel@themaw.net>
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@themaw.net>:
> 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@themaw.net>:
>> > 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@themaw.net>:
>> > > > 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@gmail.com>
>> > > > > ---
>> > > > > 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
next prev parent reply other threads:[~2016-07-21 18:12 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-10 8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
2016-07-10 8:07 ` [PATCH 02/12] autofs: Don't fail to free_dev_ioctl(param) Tomohiro Kusumi
2016-07-10 8:07 ` [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value Tomohiro Kusumi
2016-07-19 1:34 ` Ian Kent
2016-07-19 19:29 ` Tomohiro Kusumi
2016-07-20 0:47 ` Ian Kent
2016-07-20 17:53 ` Tomohiro Kusumi
2016-07-21 2:02 ` Ian Kent
2016-07-21 18:12 ` Tomohiro Kusumi [this message]
2016-07-10 8:07 ` [PATCH 04/12] autofs: Change _LINUX_AUTO_FS4_H to _UAPI_LINUX_AUTO_FS4_H Tomohiro Kusumi
2016-07-19 1:40 ` Ian Kent
2016-07-10 8:07 ` [PATCH 05/12] autofs: Remove AUTOFS_DEVID_LEN Tomohiro Kusumi
2016-07-10 8:07 ` [PATCH 06/12] autofs: Fix Documentation regarding devid on ioctl Tomohiro Kusumi
2016-07-10 8:07 ` [PATCH 07/12] autofs: Update struct autofs_dev_ioctl in Documentation Tomohiro Kusumi
2016-07-10 8:07 ` [PATCH 08/12] autofs: Fix pr_debug() message Tomohiro Kusumi
2016-07-10 8:07 ` [PATCH 09/12] autofs: Fix wrong file/function/macro names in comments Tomohiro Kusumi
2016-07-10 8:07 ` [PATCH 10/12] autofs: Add autofs_dev_ioctl_version() for AUTOFS_DEV_IOCTL_VERSION_CMD Tomohiro Kusumi
2016-07-10 8:07 ` [PATCH 11/12] autofs: Fix print format for ioctl warning message Tomohiro Kusumi
2016-07-10 8:07 ` [PATCH 12/12] autofs: Move inclusion of linux/limits.h to uapi Tomohiro Kusumi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHndrBmA97DayAxH8RLzL4qGz0aktWCLTtxHJ32medYsPgSwgg@mail.gmail.com \
--to=kusumi.tomohiro@gmail.com \
--cc=autofs@vger.kernel.org \
--cc=raven@themaw.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).