Re: [PATCH 1/6] libfshelp_rlock

[Svante Signell]

On Tue, 2016-12-20 at 02:03 +0100, Samuel Thibault wrote:
> Hello,
> 
> Svante Signell, on Thu 15 Dec 2016 12:31:55 +0100, wrote:
> > On Thu, 2015-03-05 at 02:34 +0100, Samuel Thibault wrote:
> > > 
> No, I'm unsure what we want: do we trust the FS server as to provide
> the proper l_pid, just like we trust it to return proper file owners?
> I guess we might not have the choice actually, but I'm perhaps wrong.
> People, any thoughts on this?  Since the locker process is not involved
> at all when another process calls GETLK, I don't think there is a way
> to make the FS server have something to *prove* to the process calling
> GETLK that a given pid is the locker of a given range?  A way I could
> see would be to implement a trusted lock-auth server that FS servers
> talk with, the FS server would get a record-lock-identity token from it,
> and use an rendez-vous handshake to prove to the process calling GETLK
> that it has that token.

Earlier we talked about a proc_{user,server}_identify RPC, solving the pid
problem here, as well as the locks not being held with forks. And also the pid
identification for SCM_CREDS. Have you changed your mind?

> Anyway, really that's for later, the implementation for now won't allow
> the server to return anything meaningful in l_pid, so this is code
> that will actually never get run for now, so rather than try to write
> something which might actually reveal to be wrong, let's for now just
> return a plain ENOSYS for GETLK.

Why? file_record_lock reports that a lock is taken by another process, but
cannot give the PID, and reports -1 instead. The pid reporting is not 
fool-proof 
anyway, e.g. man fcntl(2) says abot F_GETLK:
Note  that  the returned  information may already be out of date by the time the
caller inspects it.
What potential negative effects do yo see here?

Additionally, when there is no process already locking F_GETLK correctly reports
now when a lock can be taken.

> > Which RPC and which token name are you thinking of here?
> 
> I mean the __file_record_lock RPC of course: "I want a per-process
> lock", what else could it be?  For now in your patch it only provides
> the section of the file to be locked.  When we'll implement a
> per-process lock, we'll need to say something more than just the
> section, so rather than then defining yet another RPC, let's just pave
> the way already by adding a for-now-unused port argument which will
> later be used as a token.

See above about proc_{user,server}_identify RPC.

> > > - later, we'll implement proper per-process locks, by using the token as
> > > an
> > >   identifier of the process.
> > 
> > Which token?
> 
> The token passed by the locker process when calling __file_record_lock
> for per-process lock.  That's really like the ref port of
> io_reauthenticate: when the locker process calls __file_record_lock
> for a per-process lock, it also provides to the RPC a send right
> for a new port.  It send passes a send right for the same port to
> proc_user_identify.  The FS server calls proc_server_identify with the
> received send right, and the proc server returns to the FS server the
> pid of the process.  The FS server can now trust that pid information
> and store it internally.

Again, I don't see the need to extend the file_record_lock RPC by another
argument. Maybe that's just me.

> > > - we'll then be able to implement GETLK, and in case a per-open lock was
> > >   taken, we'll put -1 in l_pid, like BSD does.
> > 
> > Please explain!
> 
> Please be polite.  Asking it this way just makes me want to stop my mail
> here...

Where is the impoliteness here? Is it the exclamation mark?

> Once __file_record_lock has the token parameter (which for now will
> just be NUL), the FS server can know whether the lock was requested
> per-process, or per-open, and when available record the pid along the
> lock. When GETLK is called, it becomes trivial for the FS server to
> either return l_pid = -1 for the per-open case, or return the l_pid for
> the per-process case.
> 
> But anyway, as I said that's for later. Just make it a mach_port_send_t
> for now, just like in io_reauthenticate, and we'll use it later.
> 
> > > - when per-opened file record locks get standardized, we can trivially
> > >   implement it.
> > 
> > See above.
> 
> See above: it'll just be a matter of passing a NUL token.

Sorry, can you give a naming of that token in the RPC definition.

We have now:
routine file_record_lock (
       file: file_t;
       RPT
       cmd: int;
       inout flock64: flock_t);

And I assume that calls in the implementations in libdiskfs/etc will use the
argument NUL?