Skip to content

Commit

Permalink
Revert "binfmt_misc: cleanup on filesystem umount"
Browse files Browse the repository at this point in the history 
This reverts commit a43edc7.

This change is causing failure with docker multirarch run, customers
are getting exec format error, reverting it as a mitigation

Signed-off-by: Mahmoud Adam <[email protected]>
  • Loading branch information
@paniakin-aws @prati0100
paniakin-aws authored and prati0100 committed Jan 30, 2025
1 parent fc4573c commit a9c078c
Showing 1 changed file with 48 additions and 168 deletions.
216 changes: 48 additions & 168 deletions fs/binfmt_misc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,12 @@ typedef struct {
char *name;
struct dentry *dentry;
struct file *interp_file;
refcount_t users; /* sync removal with load_misc_binary() */
} Node;

static DEFINE_RWLOCK(entries_lock);
static struct file_system_type bm_fs_type;
static struct vfsmount *bm_mnt;
static int entry_count;

/*
* Max length of the register string. Determined by:
Expand All @@ -81,23 +82,19 @@ static struct file_system_type bm_fs_type;
*/
#define MAX_REGISTER_LENGTH 1920

/**
* search_binfmt_handler - search for a binary handler for @bprm
* @misc: handle to binfmt_misc instance
* @bprm: binary for which we are looking for a handler
*
* Search for a binary type handler for @bprm in the list of registered binary
* type handlers.
*
* Return: binary type list entry on success, NULL on failure
/*
* Check if we support the binfmt
* if we do, return the node, else NULL
* locking is done in load_misc_binary
*/
static Node *search_binfmt_handler(struct linux_binprm *bprm)
static Node *check_file(struct linux_binprm *bprm)
{
char *p = strrchr(bprm->interp, '.');
Node *e;
struct list_head *l;

/* Walk all the registered handlers. */
list_for_each_entry(e, &entries, list) {
list_for_each(l, &entries) {
Node *e = list_entry(l, Node, list);
char *s;
int j;

Expand Down Expand Up @@ -126,49 +123,9 @@ static Node *search_binfmt_handler(struct linux_binprm *bprm)
if (j == e->size)
return e;
}

return NULL;
}

/**
* get_binfmt_handler - try to find a binary type handler
* @misc: handle to binfmt_misc instance
* @bprm: binary for which we are looking for a handler
*
* Try to find a binfmt handler for the binary type. If one is found take a
* reference to protect against removal via bm_{entry,status}_write().
*
* Return: binary type list entry on success, NULL on failure
*/
static Node *get_binfmt_handler(struct linux_binprm *bprm)
{
Node *e;

read_lock(&entries_lock);
e = search_binfmt_handler(bprm);
if (e)
refcount_inc(&e->users);
read_unlock(&entries_lock);
return e;
}

/**
* put_binfmt_handler - put binary handler node
* @e: node to put
*
* Free node syncing with load_misc_binary() and defer final free to
* load_misc_binary() in case it is using the binary type handler we were
* requested to remove.
*/
static void put_binfmt_handler(Node *e)
{
if (refcount_dec_and_test(&e->users)) {
if (e->flags & MISC_FMT_OPEN_FILE)
filp_close(e->interp_file, NULL);
kfree(e);
}
}

/*
* the loader itself
*/
Expand All @@ -182,7 +139,12 @@ static int load_misc_binary(struct linux_binprm *bprm)
if (!enabled)
return retval;

fmt = get_binfmt_handler(bprm);
/* to keep locking time low, we copy the interpreter string */
read_lock(&entries_lock);
fmt = check_file(bprm);
if (fmt)
dget(fmt->dentry);
read_unlock(&entries_lock);
if (!fmt)
return retval;

Expand Down Expand Up @@ -236,16 +198,7 @@ static int load_misc_binary(struct linux_binprm *bprm)

retval = 0;
ret:

/*
* If we actually put the node here all concurrent calls to
* load_misc_binary() will have finished. We also know
* that for the refcount to be zero ->evict_inode() must have removed
* the node to be deleted from the list. All that is left for us is to
* close and free.
*/
put_binfmt_handler(fmt);

dput(fmt->dentry);
return retval;
}

Expand Down Expand Up @@ -600,90 +553,30 @@ static struct inode *bm_get_inode(struct super_block *sb, int mode)
return inode;
}

/**
* bm_evict_inode - cleanup data associated with @inode
* @inode: inode to which the data is attached
*
* Cleanup the binary type handler data associated with @inode if a binary type
* entry is removed or the filesystem is unmounted and the super block is
* shutdown.
*
* If the ->evict call was not caused by a super block shutdown but by a write
* to remove the entry or all entries via bm_{entry,status}_write() the entry
* will have already been removed from the list. We keep the list_empty() check
* to make that explicit.
*/
static void bm_evict_inode(struct inode *inode)
{
Node *e = inode->i_private;

clear_inode(inode);
if (e && e->flags & MISC_FMT_OPEN_FILE)
filp_close(e->interp_file, NULL);

if (e) {
write_lock(&entries_lock);
if (!list_empty(&e->list))
list_del_init(&e->list);
write_unlock(&entries_lock);
put_binfmt_handler(e);
}
clear_inode(inode);
kfree(e);
}

/**
* unlink_binfmt_dentry - remove the dentry for the binary type handler
* @dentry: dentry associated with the binary type handler
*
* Do the actual filesystem work to remove a dentry for a registered binary
* type handler. Since binfmt_misc only allows simple files to be created
* directly under the root dentry of the filesystem we ensure that we are
* indeed passed a dentry directly beneath the root dentry, that the inode
* associated with the root dentry is locked, and that it is a regular file we
* are asked to remove.
*/
static void unlink_binfmt_dentry(struct dentry *dentry)
static void kill_node(Node *e)
{
struct dentry *parent = dentry->d_parent;
struct inode *inode, *parent_inode;

/* All entries are immediate descendants of the root dentry. */
if (WARN_ON_ONCE(dentry->d_sb->s_root != parent))
return;

/* We only expect to be called on regular files. */
inode = d_inode(dentry);
if (WARN_ON_ONCE(!S_ISREG(inode->i_mode)))
return;

/* The parent inode must be locked. */
parent_inode = d_inode(parent);
if (WARN_ON_ONCE(!inode_is_locked(parent_inode)))
return;

if (simple_positive(dentry)) {
dget(dentry);
simple_unlink(parent_inode, dentry);
d_delete(dentry);
dput(dentry);
}
}
struct dentry *dentry;

/**
* remove_binfmt_handler - remove a binary type handler
* @misc: handle to binfmt_misc instance
* @e: binary type handler to remove
*
* Remove a binary type handler from the list of binary type handlers and
* remove its associated dentry. This is called from
* binfmt_{entry,status}_write(). In the future, we might want to think about
* adding a proper ->unlink() method to binfmt_misc instead of forcing caller's
* to use writes to files in order to delete binary type handlers. But it has
* worked for so long that it's not a pressing issue.
*/
static void remove_binfmt_handler(Node *e)
{
write_lock(&entries_lock);
list_del_init(&e->list);
write_unlock(&entries_lock);
unlink_binfmt_dentry(e->dentry);

dentry = e->dentry;
drop_nlink(d_inode(dentry));
d_drop(dentry);
dput(dentry);
simple_release_fs(&bm_mnt, &entry_count);
}

/* /<entry> */
Expand All @@ -710,8 +603,8 @@ bm_entry_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
struct inode *inode = file_inode(file);
Node *e = inode->i_private;
struct dentry *root;
Node *e = file_inode(file)->i_private;
int res = parse_command(buffer, count);

switch (res) {
Expand All @@ -725,22 +618,13 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
break;
case 3:
/* Delete this handler. */
inode = d_inode(inode->i_sb->s_root);
inode_lock(inode);
root = file_inode(file)->i_sb->s_root;
inode_lock(d_inode(root));

/*
* In order to add new element or remove elements from the list
* via bm_{entry,register,status}_write() inode_lock() on the
* root inode must be held.
* The lock is exclusive ensuring that the list can't be
* modified. Only load_misc_binary() can access but does so
* read-only. So we only need to take the write lock when we
* actually remove the entry from the list.
*/
if (!list_empty(&e->list))
remove_binfmt_handler(e);
kill_node(e);

inode_unlock(inode);
inode_unlock(d_inode(root));
break;
default:
return res;
Expand Down Expand Up @@ -799,7 +683,13 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (!inode)
goto out2;

refcount_set(&e->users, 1);
err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
if (err) {
iput(inode);
inode = NULL;
goto out2;
}

e->dentry = dget(dentry);
inode->i_private = e;
inode->i_fop = &bm_entry_operations;
Expand Down Expand Up @@ -843,8 +733,7 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
int res = parse_command(buffer, count);
Node *e, *next;
struct inode *inode;
struct dentry *root;

switch (res) {
case 1:
Expand All @@ -857,22 +746,13 @@ static ssize_t bm_status_write(struct file *file, const char __user *buffer,
break;
case 3:
/* Delete all handlers. */
inode = d_inode(file_inode(file)->i_sb->s_root);
inode_lock(inode);
root = file_inode(file)->i_sb->s_root;
inode_lock(d_inode(root));

/*
* In order to add new element or remove elements from the list
* via bm_{entry,register,status}_write() inode_lock() on the
* root inode must be held.
* The lock is exclusive ensuring that the list can't be
* modified. Only load_misc_binary() can access but does so
* read-only. So we only need to take the write lock when we
* actually remove the entry from the list.
*/
list_for_each_entry_safe(e, next, &entries, list)
remove_binfmt_handler(e);
while (!list_empty(&entries))
kill_node(list_first_entry(&entries, Node, list));

inode_unlock(inode);
inode_unlock(d_inode(root));
break;
default:
return res;
Expand Down

0 comments on commit a9c078c

Please sign in to comment.