checkpoint/restart: refuse checkpoint on detached file
PATCH 1/1 checkpoint/restart: refuse checkpoint on detached file
So Dave brought up the problem of checkpointing a task which has an open fd to a file which is under a directory which has been lazily unmounted.
Now long-term I suspect we will want to do something like figure out another path to the file which restart can use, or, if we've lost the last ref to the file then treat it as a deleted private file... but in the meantime we should refuse to checkpoint such a task, since it won't be restartable.
On irc we'd talked about catching umount -l and balancing it with the final mntput, and setting the uncheckpointable flag on the whole namespace in the meantime.
As an alternative, I thought it might be good to just check each file for whether it's under a unmounted vfsmount. The reason I think this is a good approach is that if we try to actually solve the problem for real, we'll want to do these steps anyway.
Opinions?
From 4a39c479498bdce027320a1cc91a093ed2fb8450 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue-r/[email protected]>
Date: Thu, 4 Dec 2008 20:16:22 -0800
Subject: [PATCH 1/1] checkpoint/restart: refuse checkpoint on detached file
If a file is on an fs which has been detached, refuse the checkpoint (for now).
--- checkpoint/ckpt_file.c | 21 +++++++++++++++++++++ fs/namespace.c | 24 ++++++++++++++++++++++++ include/linux/dcache.h | 1 + 3 files changed, 46 insertions(+), 0 deletions(-)
checkpoint/ckpt_file.c +21 -0
@@ -14,6 +14,9 @@#include <linux/fdtable.h> #include <linux/checkpoint.h> #include <linux/checkpoint_hdr.h> +#include <linux/fs.h> +#include <linux/mount.h> +#include <linux/dcache.h> #include "checkpoint_file.h"
@@ -123,6 +126,18 @@ static int cr_write_fd_data(struct cr_ctx *ctx, struct file *file, int parent)return cr_write_fname (ctx, &file->f_path, ctx->vfsroot); } +int file_in_detached_tree (struct file *file) +{ +struct vfsmount *mnt = file->f_path.mnt; + +while (mnt->mnt_parent != mnt) { +if (!is_still_mounted (mnt)) +return 1 ; + mnt = mnt->mnt_parent; + } +return 0 ; +} +/** * cr_write_fd_ent - dump the state of a given file descriptor * @ctx: checkpoint context
@@ -158,6 +173,12 @@ cr_write_fd_ent(struct cr_ctx *ctx, struct files_struct *files, int fd) goto out; } +/* Make sure this isn't under some detached tree */ +if (file_in_detached_tree (file)) { + ret = -EBADF ; + goto out; + } + new =cr_obj_add_ptr (ctx, file, &objref,CR_OBJ_FILE ,0 );cr_debug ("fd %d objref %d file %p c-o-e %d)\n" , fd, objref, file, coe);
fs/namespace.c +24 -0
@@ -438,6 +438,30 @@ struct vfsmount *__lookup_mnt(struct vfsmount *mnt, struct dentry *dentry,return found; } +int is_still_mounted (struct vfsmount *childmnt) +{ +struct vfsmount *mnt = childmnt->mnt_parent; +struct dentry *dentry = childmnt->mnt_mountpoint; +struct list_head *head = mount_hashtable +hash (mnt, dentry); +struct list_head *tmp = head; +struct vfsmount *p; +int found =0 ; + +for (;;) { + tmp = tmp->next; + p =NULL ; +if (tmp == head) +break ; + p = list_entry(tmp, struct vfsmount, mnt_hash); +if (p == childmnt) { + found =1 ; +break ; + } + } +return found; + +} +/* * lookup_mnt increments the ref count before returning * the vfsmount struct.
include/linux/dcache.h +1 -0
@@ -363,6 +363,7 @@ static inline int d_mountpoint(struct dentry *dentry)extern struct vfsmount *lookup_mnt (struct vfsmount *,struct dentry *);extern struct dentry *lookup_create (struct nameidata *nd,int is_dir); +extern int is_still_mounted (struct vfsmount *childmnt);extern int sysctl_vfs_cache_pressure;
Signature · 1 line
1.5.4.3
A message that is not in the archive (sent off-list or not yet received): 20081205044141.GA1444-r/[email protected]
checkpoint/ckpt_file.c cr_write_fd_ent(struct cr_ctx *ctx, struct · line 181
Quoted 11 lines · Serge E. Hallyn
checkpoint/ckpt_file.c +5
-0@@ -158,6 +173,12 @@ cr_write_fd_ent(struct cr_ctx *ctx, struct files_struct *files, int fd) goto out; } +/* Make sure this isn't under some detached tree */ +if (file_in_detached_tree (file)) { + ret = -EBADF ; + goto out; + }
Looks fine to me. This is racy, though. Right?
There's no locking to keep the thing mounted for the duration of the checkpoint.
-- Dave
Quoted 15 lines · Dave Hansen
checkpoint/ckpt_file.c cr_write_fd_ent(struct cr_ctx *ctx, struct · line 181
Quoted 11 lines · Serge E. Hallyn
checkpoint/ckpt_file.c +5
-0@@ -158,6 +173,12 @@ cr_write_fd_ent(struct cr_ctx *ctx, struct files_struct *files, int fd) goto out; } +/* Make sure this isn't under some detached tree */ +if (file_in_detached_tree (file)) { + ret = -EBADF ; + goto out; + }Looks fine to me. This is racy, though. Right?
Quoted 2 lines
There's no locking to keep the thing mounted for the duration of the checkpoint.
Oh, hahah, yeah. We have the file pinned so we're not going to lose any vfsmnt/dentries, but you're right, someone else could come along and umount -l in the middle.
I suppose we could hold the namespace sem but it doesn't seem worth it and could deadlock.
Patch withdrawn for now :)
-serge
A message that is not in the archive (sent off-list or not yet received): 20081205224643.GA29599-r/[email protected]
Quoted 19 lines · Serge E. Hallyn
Quoted 15 lines · Dave Hansen
checkpoint/ckpt_file.c cr_write_fd_ent(struct cr_ctx *ctx, struct · line 181
Quoted 11 lines · Serge E. Hallyn
checkpoint/ckpt_file.c +5
-0@@ -158,6 +173,12 @@ cr_write_fd_ent(struct cr_ctx *ctx, struct files_struct *files, int fd) goto out; } +/* Make sure this isn't under some detached tree */ +if (file_in_detached_tree (file)) { + ret = -EBADF ; + goto out; + }Looks fine to me. This is racy, though. Right?
Quoted 2 lines
There's no locking to keep the thing mounted for the duration of the checkpoint.
Quoted 8 lines
Oh, hahah, yeah. We have the file pinned so we're not going to lose any vfsmnt/dentries, but you're right, someone else could come along and umount -l in the middle.
I suppose we could hold the namespace sem but it doesn't seem worth it and could deadlock.
Patch withdrawn for now :)
Well, it is better than nothing. We don't have to worry about people messing with it if we have complete control over the entire mnt_namespace.
-- Dave