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;
> }
> (...)
Yes, that does leak descriptors for the case where it was opened in the
function. I see the same problem in umount_autofs_offset() too.
>
> 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.
That would be best but the old entry might have an active mount which
means it needs to be kept around.
>
> 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?
Probably should do that before the umount since it prevents any further
requests from being done and completes any pending requests.
> - 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?
Not sure, I'll need to have a closer look.
If it has an open file handle it probably has an active mount which
means the descriptor should remain open for subsequent expires to work.
>
> 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
