On Tue, 2012-08-14 at 22:28 -0300, Leonardo Chiquitto wrote:
> Hello,
>
> A customer reported that AutoFS may leak file descriptors when some
> maps are modified and the daemon reloaded. I'm able to reproduce the
> problem on 5.0.7 by following these steps:
>
> 1. Configure a simple direct mount:
>
> # cat /etc/auto.master
> /- /etc/auto.direct
>
> # cat /etc/auto.direct
> /nfs server:/nfs
>
> 2. Start the automounter and do NOT trigger the mount
>
> 3. Replace /etc/auto.direct with:
>
> # cat /etc/auto.direct
> /nfs/1 server:/nfs
> /nfs/2 server:/nfs
>
> 4. Reload:
>
> # kill -HUP $(pidof automount)
>
> From now on, every reload will leak a file descriptor:
>
> # ls -la /proc/$(pidof automount)/fd | grep /nfs
> lr-x------ 1 root root 64 Aug 14 22:08 11 -> /nfs
> lr-x------ 1 root root 64 Aug 14 22:08 12 -> /nfs
> lr-x------ 1 root root 64 Aug 14 22:08 13 -> /nfs
> lr-x------ 1 root root 64 Aug 14 22:08 14 -> /nfs
> lr-x------ 1 root root 64 Aug 14 22:08 5 -> /nfs
>
> I've investigated the problem and discovered that the leak happens in
> do_umount_autofs_direct():
>
> int do_umount_autofs_direct(struct autofs_point *ap, struct mnt_list
> *mnts, struct mapent *me)
> {
> (...)
> if (me->ioctlfd != -1) {
> if (tree_is_mounted(mnts, me->key, MNTS_REAL)) {
> error(ap->logopt,
> "attempt to umount busy direct mount %s",
> me->key);
> return 1;
> }
> ioctlfd = me->ioctlfd;
> } else // ioctlfd == -1
> ops->open(ap->logopt, &ioctlfd, me->dev, me->key); <= we open it here
>
> if (ioctlfd >= 0) {
> unsigned int status = 1;
>
> rv = ops->askumount(ap->logopt, ioctlfd, &status);
> /// at this point, rv == 0 and status == 0
> if (rv) {
> char *estr = strerror_r(errno, buf, MAX_ERR_BUF);
> error(ap->logopt, "ioctl failed: %s", estr);
> return 1;
> } else if (!status) {
> /// at this point, ap->state == ST_READMAP
> if (ap->state != ST_SHUTDOWN_FORCE) {
> error(ap->logopt,
> "ask umount returned busy for %s",
> me->key);
> return 1; <= we return here, without closing the fd
> } else {
> me->ioctlfd = -1;
> ops->catatonic(ap->logopt, ioctlfd);
> ops->close(ap->logopt, ioctlfd);
> goto force_umount;
> }
> (...)
>
> I wrote a patch to workaround the leak because I still don't know how
> to fix the root cause, which I believe is to remove all references to the
> old map entry and unmount everything before creating the new entries.
Returning to this for another look ....
>
> There's also some open points in the workaround:
> - should we call ops->catatonic() on the ioctlfd like it's done in the other
> error paths?
Don't think so because, if we are not forcing a shutdown, the mount is
busy somehow and autofs might be able recover from the situation. So I
think the best thing we can do here is leave the mount in a
non-catatonic state.
> - I've opted to close ioctlfd only if it was opened in this function. Is
> this correct or should we close it regardless if it was opened before
> the call?
Again, don't think so because, if the descriptor has been opened
elsewhere then it is the direct (or offset) mount control descriptor and
the case here is saying we can't umount it because it is in use somehow.
Leaving an existing descriptor should allow autofs to continue to try
and expire it.
I started work on this and ended up with a fairly lengthy patch but now
I'm thinking maybe your approach is best, still not sure yet.
>
> Thanks,
> Leonardo
>
> Index: autofs-5.0.7/daemon/direct.c
> ===================================================================
> --- autofs-5.0.7.orig/daemon/direct.c
> +++ autofs-5.0.7/daemon/direct.c
> @@ -87,6 +87,7 @@ int do_umount_autofs_direct(struct autof
> struct ioctl_ops *ops = get_ioctl_ops();
> char buf[MAX_ERR_BUF];
> int ioctlfd, rv, left, retries;
> + int opened = 0;
>
> left = umount_multi(ap, me->key, 0);
> if (left) {
> @@ -103,8 +104,10 @@ int do_umount_autofs_direct(struct autof
> return 1;
> }
> ioctlfd = me->ioctlfd;
> - } else
> + } else {
> ops->open(ap->logopt, &ioctlfd, me->dev, me->key);
> + opened = 1;
> + }
>
> if (ioctlfd >= 0) {
> unsigned int status = 1;
> @@ -119,6 +122,8 @@ int do_umount_autofs_direct(struct autof
> error(ap->logopt,
> "ask umount returned busy for %s",
> me->key);
> + if (opened)
> + ops->close(ap->logopt, ioctlfd);
> return 1;
> } else {
> me->ioctlfd = -1;
> --
> To unsubscribe from this list: send the line "unsubscribe autofs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe autofs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
