Re: [PATCH 11/19] switch simple users of fdget() to CLASS(fd, ...)

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Jun 07, 2024 at 05:26:53PM +0200, Christian Brauner wrote:
> On Fri, Jun 07, 2024 at 02:59:49AM +0100, Al Viro wrote:
> > low-hanging fruit; that's another likely source of conflicts
> > over the cycle and it might make a lot of sense to split;
> > fortunately, it can be split pretty much on per-function
> > basis - chunks are independent from each other.
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> 
> I can pick conversions from you for files where I already have changes
> in the tree anyway or already have done conversion as part of other
> patches.

Some notes:

	This kind of conversions depends upon fdput(empty) being not just
a no-op, but a no-op visible to compiler.  Representation change arranges
for that.

	CLASS(...) has some misfeatures or nearly C++ level of tastelessness;
for this series I decided to try it and see how it goes, but... AFAICS in
a lot of cases it's the wrong answer.

	1.  Variable declarations belong in the beginning of block.
CLASS use invites violating that; to make things worse, in-block goto
bypassing such declaration is only caught by current clang.  Two
examples got caught by Intel buildbot in this patch, actually - one
in fs/select.c, another in ocfs2.

	2. The loss of control over the location of destructor call can
be dangerous in some cases.  Delaying it is not a problem for struct
file references (well, except for goto problems above), but there's
an opposite issue.  Example from later in this series:
	if (arg != -1) {
		struct perf_event *output_event;
		struct fd output;
		ret = perf_fget_light(arg, &output);
		if (ret)
			return ret;
		output_event = output.file->private_data;
		ret = perf_event_set_output(event, output_event);
		fdput(output);
	} else {
		ret = perf_event_set_output(event, NULL);
	}
gets converted to
	if (arg != -1) {
		struct perf_event *output_event;
		CLASS(fd, output)(arg);
		if (!is_perf_file(output))
			return -EBADF;
		output_event = fd_file(output)->private_data;
		return perf_event_set_output(event, output_event);
	} else {
		return perf_event_set_output(event, NULL);
	}

Nice, but that invites the next step, doesn't it?  Like this:

	struct perf_event *output_event = NULL;
	if (arg != -1) {
		CLASS(fd, output)(arg);
		if (!is_perf_file(output))
			return -EBADF;
		output_event = fd_file(output)->private_data;
	}
	return perf_event_set_output(event, output_event);

See the trouble here?  The last variant would be broken - the value of
file->private_data escapes the scope and can end up used after
the file gets closed.

In the original variant it's easy to see - we have an explicit
fdput() marking the place where protection disappears.  After
the conversion to implicit cleanups we lose that.

Yes, use of __cleanup can nicely shrink the source and make
some leaks less likely.  But my impression from this experiment
is that we should be very cautious with using that technics ;-/