Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained

[ebiederm@xmission.com (Eric W. Biederman)]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Sat, Jun 13, 2020 at 8:33 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Sat, Jun 13, 2020 at 7:13 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>> > I am in the middle of cleaning up exec.  Send the patches that address
>> > the issues and make this mess not a maintenance issue, and I will be
>> > happy to leave fork_usermode_blob alone.  Otherwise I plan to just
>> > remove the code for now as it is all dead at the moment.
>>
>> May be stop being a jerk first ?
>> It's a Nack to remove the code.
>
> I'm happy to work on changes, but your removal threats must stop
> before we can continue discussion. ok?

I am looking for reasons to not remove the code.  What I can't
personally do is justify spending the time fixing unused code.

The code is currently unused, and has an implementation that can be
improved.  All of which argues for removal on technical grounds.

The implementation issues are a problem for maintaining other code so
they need to be addressed, again that argues for removal on technical
grounds.

There are some asperations to use fork_usermode_blob but no commitments
that I can see to actually use this code.

If someone who cares can step up so other developers don't have to deal
with the maintenance problems, then there is no problem in keeping the
code.




Now there is one technical issue I see that has implications for how
this gets fixed.  The current implementation requires that 2 copies
of the user mode executable be kept.

int fork_usermode_blob(void *data, size_t len, struct umh_info *info);


The function fork_usermode_blob is passed an array and a length.  Today
that array is stored in .rodata.  Not in a init section where it could
be discared.

Now userspace in general and exec in particular requires the executable
to be in a mmapable file.  So fork_usermode_blob creates a mini
filesystem that only supports one file and no file names and opens
a file within it, and passes that open file to exec.

If creation of the filesystem and copying of the data can be separated
from the actual execution of the code, then there will be no need to
keep 2 copies of the executable in memory.  If the file was also given a
name there would be no need for fork_usermode_blob to open the file.
All fork_usermode_blob would need to do is make make it possible for
exec to find that file.

The implification this has for fixing the issues with exec is that once
the file has a name fork_usermode_blob no longer needs to preopen the
file and call do_execve_file.  Instead fork_usermode_blob can call
do_execve.  Which means do_execve_file and all of it's strange corner
cases can go away.

We have all of the infrastructure to decode a cpio in init/initramfs.c
so it would be practically no code at all to place the code into a
filesystem instead of just into a file at startup time.  At which
point it could be guaranteed that the section the filesystem lives in is
an init section and is not used past the point of loading it into a
filesystem.  Making the code use half the memory.

Eric