On Fri, 2018-05-25 at 01:07 +0100, David Howells wrote:
> Implement the ability for filesystems to log error, warning and
> informational messages through the fs_context. These can be extracted by
> userspace by reading from an fd created by fsopen().
>
> Error messages are prefixed with "e ", warnings with "w " and informational
> messages with "i ".
[]
> diff --git a/fs/fs_context.c b/fs/fs_context.c
[]
> +/**
> + * logfc - Log a message to a filesystem context
> + * @fc: The filesystem context to log to.
> + * @fmt: The format of the buffer.
> + */
> +void logfc(struct fs_context *fc, const char *fmt, ...)
> +{
> + static const char store_failure[] = "OOM: Can't store error string";
> + struct fc_log *log = fc->log;
> + unsigned int logsize = ARRAY_SIZE(log->buffer);
> + const char *p;
> + va_list va;
> + char *q;
> + u8 freeable, index;
> +
> + if (!log)
> + return;
> +
> + va_start(va, fmt);
> + if (!strchr(fmt, '%')) {
> + p = fmt;
> + goto unformatted_string;
> + }
> + if (strcmp(fmt, "%s") == 0) {
> + p = va_arg(va, const char *);
> + goto unformatted_string;
> + }
> +
> + q = kvasprintf(GFP_KERNEL, fmt, va);
> +copied_string:
> + if (!q)
> + goto store_failure;
> + freeable = 1;
> + goto store_string;
> +
> +unformatted_string:
> + if ((unsigned long)p >= (unsigned long)__start_rodata &&
> + (unsigned long)p < (unsigned long)__end_rodata)
> + goto const_string;
> + if (within_module_core((unsigned long)p, log->owner))
> + goto const_string;
> + q = kstrdup(p, GFP_KERNEL);
> + goto copied_string;
> +
> +store_failure:
> + p = store_failure;
> +const_string:
> + q = (char *)p;
> + freeable = 0;
> +store_string:
> + index = log->head & (logsize - 1);
> + if ((int)log->head - (int)log->tail == 8) {
> + /* The buffer is full, discard the oldest message */
> + if (log->need_free & (1 << index))
> + kfree(log->buffer[index]);
> + log->tail++;
> + }
> +
> + log->buffer[index] = q;
> + log->need_free &= ~(1 << index);
> + log->need_free |= freeable << index;
> + log->head++;
> + va_end(va);
> +}
> +EXPORT_SYMBOL(logfc);
Perhaps this could be renamed to something more obviously
associated to fs_context
[]
> diff --git a/fs/fsopen.c b/fs/fsopen.c
[]
> @@ -159,7 +159,57 @@ static ssize_t fscontext_fs_write(struct file *file,
> goto err_unlock;
> }
>
> +/*
> + * Allow the user to read back any error, warning or informational messages.
> + */
> +static ssize_t fscontext_fs_read(struct file *file,
> + char __user *_buf, size_t len, loff_t *pos)
> +{
> + struct fs_context *fc = file->private_data;
> + struct fc_log *log = fc->log;
> + struct inode *inode = file_inode(file);
> + unsigned int logsize = ARRAY_SIZE(log->buffer);
logsize isn't modified, could be removed and ARRAY_SIZE used in-place.
> + ssize_t ret;
> + char *p;
> + bool need_free;
It _looks_ like need_free isn't set by all codepaths, but
it doesn't need to be given the goto/return.
It also seems the the logic isn't straightforward here and
could be rewritten to be more obvious.
> + int index, n;
> +
> + ret = inode_lock_killable(inode);
> + if (ret < 0)
> + return ret;
> +
> + ret = -ENODATA;
> + if (log->head != log->tail) {
> + index = log->tail & (logsize - 1);
> + p = log->buffer[index];
> + need_free = log->need_free & (1 << index);
> + log->buffer[index] = NULL;
> + log->need_free &= ~(1 << index);
> + log->tail++;
> + ret = 0;
> + }
> +
> + inode_unlock(inode);
> + if (ret < 0)
> + return ret;
> +
> + ret = -EMSGSIZE;
> + n = strlen(p);
> + if (n > len)
> + goto err_free;
> + ret = -EFAULT;
> + if (copy_to_user(_buf, p, n) != 0)
> + goto err_free;
> + ret = n;
> +
> +err_free:
> + if (need_free)
> + kfree(p);
> + return ret;
> +}
> +
> const struct file_operations fscontext_fs_fops = {
> + .read = fscontext_fs_read,
> .write = fscontext_fs_write,
> .release = fscontext_fs_release,
> .llseek = no_llseek,
> @@ -330,6 +380,7 @@ SYSCALL_DEFINE5(fsopen, const char __user *, _fs_name,
> unsigned int, flags,
> struct file_system_type *fs_type;
> struct fs_context *fc;
> const char *fs_name;
> + int ret;
>
> if (!ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN))
> return -EPERM;
> @@ -353,7 +404,18 @@ SYSCALL_DEFINE5(fsopen, const char __user *, _fs_name,
> unsigned int, flags,
>
> fc->phase = FS_CONTEXT_CREATE_PARAMS;
>
> + ret = -ENOMEM;
> + fc->log = kzalloc(sizeof(*fc->log), GFP_KERNEL);
> + if (!fc->log)
> + goto err_fc;
> + refcount_set(&fc->log->usage, 1);
> + fc->log->owner = fs_type->owner;
> +
> return fsopen_create_fd(fc, flags & FSOPEN_CLOEXEC);
> +
> +err_fc:
> + put_fs_context(fc);
> + return ret;
> }
[]
> diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
>
> @@ -117,4 +119,60 @@ extern int vfs_get_super(struct fs_context *fc,
>
> extern const struct file_operations fscontext_fs_fops;
>
> +/*
> + * Mount error, warning and informational message logging. This structure is
> + * shareable between a mount and a subordinate mount.
> + */
> +struct fc_log {
> + refcount_t usage;
> + u8 head; /* Insertion index in buffer[] */
> + u8 tail; /* Removal index in buffer[] */
> + u8 need_free; /* Mask of kfree'able items in buffer[]
> */
> + struct module *owner; /* Owner module for strings that don't
> then need freeing */
> + char *buffer[8];
> +};
> +
> +extern __attribute__((format(printf, 2, 3)))
__printf(2, 3)
> +void logfc(struct fs_context *fc, const char *fmt, ...);
> +
> +/**
> + * infof - Store supplementary informational message
> + * @fc: The context in which to log the informational message
> + * @fmt: The format string
> + *
> + * Store the supplementary informational message for the process if the
> process
> + * has enabled the facility.
> + */
> +#define infof(fc, fmt, ...) ({ logfc(fc, "i "fmt, ## __VA_ARGS__); })
Why a statement expression macro and not just
#define infof(fc, fmt, ...) logfc(fc, "i " fmt, ##__VA_ARGS__)
etc...
