Re: [PATCH RFC v4 15/18] coredump: add COREDUMP_CLOSE_FILES

[Oleg Nesterov <oleg@redhat.com>]

On 09/10, Christian Brauner wrote:
>
> Add COREDUMP_CLOSE_FILES and allow a coredump server to request that the
> thread-group closes all files before creating the coredump.

Perhaps the changelog could explain the motivation for this feature...

The patch looks correct, but let me ask some question.

> +static bool coredump_close_files(struct core_state *core_state)
> +{
> +	struct files_struct *files;
> +	struct core_thread *ct;
> +
> +	files = alloc_files_struct();
> +	if (!files)
> +		return false;
> +
> +	for (ct = core_state->tasks; ct; ct = ct->next) {
> +		/* Tasks without a table such as vhost workers can be skipped. */
> +		if (!ct->task->files)
> +			continue;
> +		atomic_inc(&core_state->threads_remaining);
> +		/* ct->files holds a reference until the thread switches to it. */
> +		atomic_inc(&files->count);
> +		/* Pairs with the acquire in coredump_task_exit(). */
> +		smp_store_release(&ct->files, files);
> +		wake_up_process(ct->task);

I don't understand this smp_store_release() and smp_load_acquire() in
coredump_task_exit().

Both wake_up_process() and set_current_state() have the necessary barriers,
see the comment above set_current_state(). So it seems that we can use the
plain store/load.

But what I really can't understand is why coredump_close_files() can't just
do switch_files_struct() itself. It could do override_creds(ct->task->cred)
if this is one of the reasons.

Could you explain this please?

Oleg.