Currently F_OFD_GETLK sets the pid of the lock owner to -1.
Remove such behavior to allow getting the proper owner's pid.
This may be helpful when you want to send some message (like SIGKILL)
to the offending locker.
Signed-off-by: Stas Sergeev <[email protected]>
CC: Jeff Layton <[email protected]>
CC: Chuck Lever <[email protected]>
CC: Alexander Viro <[email protected]>
CC: Christian Brauner <[email protected]>
CC: [email protected]
CC: [email protected]
---
fs/locks.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 210766007e63..ee265e166542 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2158,8 +2158,6 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns)
pid_t vnr;
struct pid *pid;
- if (IS_OFDLCK(fl))
- return -1;
if (IS_REMOTELCK(fl))
return fl->fl_pid;
/*
--
2.39.2
Hello,
20.06.2023 15:51, Jeff Layton пишет:
> On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
>> Currently F_OFD_GETLK sets the pid of the lock owner to -1.
>> Remove such behavior to allow getting the proper owner's pid.
>> This may be helpful when you want to send some message (like SIGKILL)
>> to the offending locker.
>>
>> Signed-off-by: Stas Sergeev <[email protected]>
>>
>> CC: Jeff Layton <[email protected]>
>> CC: Chuck Lever <[email protected]>
>> CC: Alexander Viro <[email protected]>
>> CC: Christian Brauner <[email protected]>
>> CC: [email protected]
>> CC: [email protected]
>>
>> ---
>> fs/locks.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/locks.c b/fs/locks.c
>> index 210766007e63..ee265e166542 100644
>> --- a/fs/locks.c
>> +++ b/fs/locks.c
>> @@ -2158,8 +2158,6 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns)
>> pid_t vnr;
>> struct pid *pid;
>>
>> - if (IS_OFDLCK(fl))
>> - return -1;
>> if (IS_REMOTELCK(fl))
>> return fl->fl_pid;
>> /*
> NACK on this one.
>
> OFD locks are not owned by processes. They are owned by the file
> description (hence the name). Because of this, returning a pid here is
> wrong.
But fd is owned by a process.
PID has a meaning, you can send SIGKILL
to the returned PID, and the lock is clear.
Was there any reason to hide the PID at
a first place?
> This precedent comes from BSD, where flock() and POSIX locks can
> conflict. BSD returns -1 for the pid if you call F_GETLK on a file
> locked with flock(). Since OFD locks have similar ownership semantics to
> flock() locks, we use the same convention here.
OK if you insist I can drop this one and
search the PID by some other means.
Just a bit unsure what makes it so important
to overwrite the potentially useful info
with -1.
So in case you insist on that, then should
I send a v2 or can you just drop the patch
yourself?
On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
> Currently F_OFD_GETLK sets the pid of the lock owner to -1.
> Remove such behavior to allow getting the proper owner's pid.
> This may be helpful when you want to send some message (like SIGKILL)
> to the offending locker.
>
> Signed-off-by: Stas Sergeev <[email protected]>
>
> CC: Jeff Layton <[email protected]>
> CC: Chuck Lever <[email protected]>
> CC: Alexander Viro <[email protected]>
> CC: Christian Brauner <[email protected]>
> CC: [email protected]
> CC: [email protected]
>
> ---
> fs/locks.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 210766007e63..ee265e166542 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2158,8 +2158,6 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns)
> pid_t vnr;
> struct pid *pid;
>
> - if (IS_OFDLCK(fl))
> - return -1;
> if (IS_REMOTELCK(fl))
> return fl->fl_pid;
> /*
NACK on this one.
OFD locks are not owned by processes. They are owned by the file
description (hence the name). Because of this, returning a pid here is
wrong.
This precedent comes from BSD, where flock() and POSIX locks can
conflict. BSD returns -1 for the pid if you call F_GETLK on a file
locked with flock(). Since OFD locks have similar ownership semantics to
flock() locks, we use the same convention here.
Cheers,
--
Jeff Layton <[email protected]>
On Tue, 2023-06-20 at 15:57 +0500, stsp wrote:
> Hello,
>
> 20.06.2023 15:51, Jeff Layton пишет:
> > On Tue, 2023-06-20 at 14:55 +0500, Stas Sergeev wrote:
> > > Currently F_OFD_GETLK sets the pid of the lock owner to -1.
> > > Remove such behavior to allow getting the proper owner's pid.
> > > This may be helpful when you want to send some message (like SIGKILL)
> > > to the offending locker.
> > >
> > > Signed-off-by: Stas Sergeev <[email protected]>
> > >
> > > CC: Jeff Layton <[email protected]>
> > > CC: Chuck Lever <[email protected]>
> > > CC: Alexander Viro <[email protected]>
> > > CC: Christian Brauner <[email protected]>
> > > CC: [email protected]
> > > CC: [email protected]
> > >
> > > ---
> > > fs/locks.c | 2 --
> > > 1 file changed, 2 deletions(-)
> > >
> > > diff --git a/fs/locks.c b/fs/locks.c
> > > index 210766007e63..ee265e166542 100644
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -2158,8 +2158,6 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns)
> > > pid_t vnr;
> > > struct pid *pid;
> > >
> > > - if (IS_OFDLCK(fl))
> > > - return -1;
> > > if (IS_REMOTELCK(fl))
> > > return fl->fl_pid;
> > > /*
> > NACK on this one.
> >
> > OFD locks are not owned by processes. They are owned by the file
> > description (hence the name). Because of this, returning a pid here is
> > wrong.
>
> But fd is owned by a process.
No, it isn't.
fd's can be handed off between processes via unix descriptors.
Multithreaded processes are also a bit of a gray area here: Suppose I
open a file and set an OFD lock on it in one task, and then let that
task exit while the file is still open. What should l_pid say in that
case?
> PID has a meaning, you can send SIGKILL
> to the returned PID, and the lock is clear.
> Was there any reason to hide the PID at
> a first place?
>
Yes, because OFD locks are not tied to a pid in the same way that
traditional POSIX locks are.
>
> > This precedent comes from BSD, where flock() and POSIX locks can
> > conflict. BSD returns -1 for the pid if you call F_GETLK on a file
> > locked with flock(). Since OFD locks have similar ownership semantics to
> > flock() locks, we use the same convention here.
> OK if you insist I can drop this one and
> search the PID by some other means.
> Just a bit unsure what makes it so important
> to overwrite the potentially useful info
> with -1.
>
> So in case you insist on that, then should
> I send a v2 or can you just drop the patch
> yourself?
--
Jeff Layton <[email protected]>
20.06.2023 16:12, Jeff Layton пишет:
> Multithreaded processes are also a bit of a gray area here: Suppose I
> open a file and set an OFD lock on it in one task, and then let that
> task exit while the file is still open. What should l_pid say in that
> case?
If by the "task" you mean a process, then
the result should be no lock at all.
If you mean the thread exit, then I would
expect l_pid to contain tgid, in which case
it will still point to the valid pid.
Or do you mean l_pid contains tid?
Checking... no, l_pid contains tgid.
So I don't really see the problem you are
pointing with the above example, could
you please clarify?
On Tue, 2023-06-20 at 16:45 +0500, stsp wrote:
> 20.06.2023 16:12, Jeff Layton пишет:
> > Multithreaded processes are also a bit of a gray area here: Suppose I
> > open a file and set an OFD lock on it in one task, and then let that
> > task exit while the file is still open. What should l_pid say in that
> > case?
>
> If by the "task" you mean a process, then
> the result should be no lock at all.
> If you mean the thread exit, then I would
> expect l_pid to contain tgid, in which case
> it will still point to the valid pid.
> Or do you mean l_pid contains tid?
> Checking... no, l_pid contains tgid.
> So I don't really see the problem you are
> pointing with the above example, could
> you please clarify?
>
Suppose I start a process (call it pid 100), and then spawn a thread
(101). I then have 101 open a file and set an OFD lock on it (such that
the resulting fl_pid field in the file_lock is set to 101). Now, join
the thread so that task 101 exits.
Wait a while, and eventually pid 101 will be recycled, so that now there
is a new task 101 that is unrelated to the process above. Another
(unrelated) task then calls F_GETLK on the file and sees that lock. Its
pid is now set to 101 and sends SIGKILL to it, killing an unrelated
process.
That's just one example, of course. The underlying problem is that OFD
locks are not owned by processes in the same way that traditional POSIX
locks are, so reporting a pid there is unreliable, at best.
--
Jeff Layton <[email protected]>
20.06.2023 17:02, Jeff Layton пишет:
> Suppose I start a process (call it pid 100), and then spawn a thread
> (101). I then have 101 open a file and set an OFD lock on it (such that
> the resulting fl_pid field in the file_lock is set to 101).
How come?
There are multiple places in locks.c
with this line:
fl->fl_pid = current->tgid;
And I've yet to see the line like:
fl->fl_pid = current->pid;
Its simply not there.
No, we put tgid into l_pid!
tgid will still be 100, no matter how
many threads you spawn or destroy.
Or what am I misseng?
> That's just one example, of course. The underlying problem is that OFD
> locks are not owned by processes in the same way that traditional POSIX
> locks are, so reporting a pid there is unreliable, at best.
But we report tgid.
It doesn't depend on threads.
I don't understand. :)
On Tue, 2023-06-20 at 17:34 +0500, stsp wrote:
> 20.06.2023 17:02, Jeff Layton пишет:
> > Suppose I start a process (call it pid 100), and then spawn a thread
> > (101). I then have 101 open a file and set an OFD lock on it (such that
> > the resulting fl_pid field in the file_lock is set to 101).
>
> How come?
> There are multiple places in locks.c
> with this line:
> fl->fl_pid = current->tgid;
>
> And I've yet to see the line like:
> fl->fl_pid = current->pid;
> Its simply not there.
>
> No, we put tgid into l_pid!
> tgid will still be 100, no matter how
> many threads you spawn or destroy.
> Or what am I misseng?
>
>
> > That's just one example, of course. The underlying problem is that OFD
> > locks are not owned by processes in the same way that traditional POSIX
> > locks are, so reporting a pid there is unreliable, at best.
> But we report tgid.
> It doesn't depend on threads.
> I don't understand. :)
Good point. I had forgotten that we stuffed the l_pid in there. So in
principle, that example would be OK. But...there is still the problem of
passing file descriptors via unix sockets.
The bottom line is that these locks are specifically not owned by a
process, so returning the l_pid field is unreliable (at best). There is
no guarantee that the pid returned will still represent the task that
set the lock.
You may want to review this article. They're called "File-private" locks
here, but the name was later changed to "Open file description" (OFD)
locks:
https://lwn.net/Articles/586904/
The rationale for why -1 is reported is noted there.
--
Jeff Layton <[email protected]>
On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
> Though it will, for sure, represent the
> task that _owns_ the lock.
No, it *DOESN'T*. I can open a file, SCM_RIGHTS pass it to another task
and then exit. Now the only owner of that lock is the recipient ...
who may not even have received the fd yet.
20.06.2023 18:19, Jeff Layton пишет:
> The bottom line is that these locks are specifically not owned by a
> process, so returning the l_pid field is unreliable (at best). There is
> no guarantee that the pid returned will still represent the task that
> set the lock.
Though it will, for sure, represent the
task that _owns_ the lock.
> You may want to review this article. They're called "File-private" locks
> here, but the name was later changed to "Open file description" (OFD)
> locks:
>
> https://lwn.net/Articles/586904/
>
> The rationale for why -1 is reported is noted there.
Well, they point to fork() and SCM_RIGHTS.
Yes, these 2 beasts can make the same lock
owned by more than one process.
Yet l_pid returned, is going to be always valid:
it will still represent one of the valid owners.
So my call is to be brave and just re-consider
the conclusion of that article, made 10 years
ago! :)
Of course if returning just 1 of possibly multiple
owners is a problem, then oh well, I'll drop
this patch.
20.06.2023 18:46, Matthew Wilcox пишет:
> On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
>> Though it will, for sure, represent the
>> task that _owns_ the lock.
> No, it *DOESN'T*. I can open a file, SCM_RIGHTS pass it to another task
> and then exit. Now the only owner of that lock is the recipient ...
Won't I get the recipient's pid in an
l_pid then?
On Tue, 2023-06-20 at 18:39 +0500, stsp wrote:
> 20.06.2023 18:19, Jeff Layton пишет:
> > The bottom line is that these locks are specifically not owned by a
> > process, so returning the l_pid field is unreliable (at best). There is
> > no guarantee that the pid returned will still represent the task that
> > set the lock.
>
> Though it will, for sure, represent the
> task that _owns_ the lock.
>
> > You may want to review this article. They're called "File-private" locks
> > here, but the name was later changed to "Open file description" (OFD)
> > locks:
> >
> > https://lwn.net/Articles/586904/
> >
> > The rationale for why -1 is reported is noted there.
> Well, they point to fork() and SCM_RIGHTS.
> Yes, these 2 beasts can make the same lock
> owned by more than one process.
> Yet l_pid returned, is going to be always valid:
> it will still represent one of the valid owners.
No, it won't. The l_pid field is populated from the file_lock->fl_pid.
That field is set when the lock is set, and never updated. So it's quite
possible for F_GETLK to return the pid of a process that no longer
exists.
In principle, we could try to address that by changing how we track lock
ownership, but that's a fairly major overhaul, and I'm not clear on any
use-cases where that matters.
> So my call is to be brave and just re-consider
> the conclusion of that article, made 10 years
> ago! :)
>
I think my foot has too many bullet wounds for that sort of bravery.
> Of course if returning just 1 of possibly multiple
> owners is a problem, then oh well, I'll drop
> this patch.
--
Jeff Layton <[email protected]>
On Tue, Jun 20, 2023 at 06:47:31PM +0500, stsp wrote:
>
> 20.06.2023 18:46, Matthew Wilcox пишет:
> > On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
> > > Though it will, for sure, represent the
> > > task that _owns_ the lock.
> > No, it *DOESN'T*. I can open a file, SCM_RIGHTS pass it to another task
> > and then exit. Now the only owner of that lock is the recipient ...
> Won't I get the recipient's pid in an
> l_pid then?
You snipped the part where I pointed out that at times there can be
_no_ task that owns it. open a fd, set the lock, pass the fd to another
task, exit. until that task calls recvmsg(), no task owns it.
20.06.2023 19:36, Matthew Wilcox пишет:
> On Tue, Jun 20, 2023 at 06:47:31PM +0500, stsp wrote:
>> 20.06.2023 18:46, Matthew Wilcox пишет:
>>> On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
>>>> Though it will, for sure, represent the
>>>> task that _owns_ the lock.
>>> No, it *DOESN'T*. I can open a file, SCM_RIGHTS pass it to another task
>>> and then exit. Now the only owner of that lock is the recipient ...
>> Won't I get the recipient's pid in an
>> l_pid then?
> You snipped the part where I pointed out that at times there can be
> _no_ task that owns it. open a fd, set the lock, pass the fd to another
> task, exit. until that task calls recvmsg(), no task owns it.
Hmm, interesting case.
So at least it seems if recipient also exits,
then the untransferred fd gets closed.
Does this mean, by any chance, that the
recipient actually owns an fd before
recvmsg() is done?
On Tue, Jun 20, 2023 at 08:45:21PM +0500, stsp wrote:
>
> 20.06.2023 19:36, Matthew Wilcox пишет:
> > On Tue, Jun 20, 2023 at 06:47:31PM +0500, stsp wrote:
> > > 20.06.2023 18:46, Matthew Wilcox пишет:
> > > > On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
> > > > > Though it will, for sure, represent the
> > > > > task that _owns_ the lock.
> > > > No, it *DOESN'T*. I can open a file, SCM_RIGHTS pass it to another task
> > > > and then exit. Now the only owner of that lock is the recipient ...
> > > Won't I get the recipient's pid in an
> > > l_pid then?
> > You snipped the part where I pointed out that at times there can be
> > _no_ task that owns it. open a fd, set the lock, pass the fd to another
> > task, exit. until that task calls recvmsg(), no task owns it.
> Hmm, interesting case.
> So at least it seems if recipient also exits,
> then the untransferred fd gets closed.
yes, pretty sure this is done by garbage collection in the unix socket
handling code, though i've never looked at it. it's done that way
because annoying people can do things like open two sockets and send the
fd of each to the other, then exit.
> Does this mean, by any chance, that the
> recipient actually owns an fd before
> recvmsg() is done?
no, it's not in their fd table. they don't own it.
20.06.2023 22:05, Matthew Wilcox пишет:
>> Does this mean, by any chance, that the
>> recipient actually owns an fd before
>> recvmsg() is done?
> no, it's not in their fd table. they don't own it.
OK, thanks for showing this pathological
case. Let me just note that this changes
nothing at all. :)
The important thing to note here is that
any lock query is race-prone: locks can
come and go at any time. So if you need
some sequence of operations, you need
to employ some global locking for that.
I use flock(LOCK_EX) on the same fd, before
doing F_OFD_GETLK, and I do flock(LOCK_UN)
only when the entire sequence of operations
is completed. And I do the same on an
F_OFD_SETLK's side to guarantee the
atomicity. You can't do it otherwise,
it would be race-prone.
So given the above, the only thing we
need for l_pid consistency is for the
"donor" process to put LOCK_EX on an
fd before doing SCM_RIGHTS, and the
recipient should do LOCK_UN. Then
the other side, which also uses LOCK_EX,
will never see the owner-less state.
And as for the kernel's POV, l_pid should
be set to -1 only when there is no owner,
like in an example you mentioned.
20.06.2023 18:58, Jeff Layton пишет:
> No, it won't. The l_pid field is populated from the file_lock->fl_pid.
> That field is set when the lock is set, and never updated. So it's quite
> possible for F_GETLK to return the pid of a process that no longer
> exists.
>
> In principle, we could try to address that by changing how we track lock
> ownership, but that's a fairly major overhaul, and I'm not clear on any
> use-cases where that matters.
OK, in this case I'll just put a comments
into the code, summarizing the info I got
from you and Matthew.
Thanks guys for all the info, its very helpful.
Now I only need to convert the current
"fundamental problem" attitude into a "not
implemented yet" via the code comment.
>> So my call is to be brave and just re-consider
>> the conclusion of that article, made 10 years
>> ago! :)
>>
> I think my foot has too many bullet wounds for that sort of bravery.
I am perfectly fine with leaving this thing
unimplemented. But what really bothers
me is the posix proposal, which I think was
done. Please tell me it allows fixing fl_pid
in the future (rather than to mandate -1),
and I am calm.
On Wed, 2023-06-21 at 11:57 +0500, stsp wrote:
> 20.06.2023 18:58, Jeff Layton пишет:
> > No, it won't. The l_pid field is populated from the file_lock->fl_pid.
> > That field is set when the lock is set, and never updated. So it's quite
> > possible for F_GETLK to return the pid of a process that no longer
> > exists.
> >
> > In principle, we could try to address that by changing how we track lock
> > ownership, but that's a fairly major overhaul, and I'm not clear on any
> > use-cases where that matters.
>
> OK, in this case I'll just put a comments
> into the code, summarizing the info I got
> from you and Matthew.
> Thanks guys for all the info, its very helpful.
>
> Now I only need to convert the current
> "fundamental problem" attitude into a "not
> implemented yet" via the code comment.
>
>
> > > So my call is to be brave and just re-consider
> > > the conclusion of that article, made 10 years
> > > ago! :)
> > >
> > I think my foot has too many bullet wounds for that sort of bravery.
> I am perfectly fine with leaving this thing
> unimplemented. But what really bothers
> me is the posix proposal, which I think was
> done. Please tell me it allows fixing fl_pid
> in the future (rather than to mandate -1),
> and I am calm.
I don't think we can change this at this point.
The bottom line (again) is that OFD locks are owned by the file
descriptor (much like with flock()), and since file descriptors can be
shared across multiple process it's impossible to say that some single
process owns it.
--
Jeff Layton <[email protected]>
21.06.2023 15:35, Jeff Layton пишет:
> I don't think we can change this at this point.
>
> The bottom line (again) is that OFD locks are owned by the file
> descriptor (much like with flock()), and since file descriptors can be
> shared across multiple process it's impossible to say that some single
> process owns it.
What's the problem with 2 owners?
Can't you get one of them, rather than
meaningless -1?
Compare this situation with read locks.
They can overlap, so when you get an
info about a read lock (except for the
new F_UNLCK case), you get the info
about *some* of the locks in that range.
In the case of multiple owners, you
likewise get the info about about some
owner. If you iteratively send them a
"please release this lock" message
(eg in a form of SIGKILL), then you
traverse all, and end up with the
lock-free area.
Is there really any problem here?
21.06.2023 16:05, Jeff Layton пишет:
> Yes. Ambiguous answers are worse than none at all.
But same for read locks, when you
query them with F_OFD_GETLK.
It doesn't sound ambiguous to me,
you get the valid owner, and you can
iterate them if you kill them in a
process (same as for read locks).
> What problem are you trying to solve by having F_OFD_GETLK report a pid?
Just a way to abruptly kill offending lockers,
as that most likely means the process hanged
(I have a 3rd party code that drives the locking,
so it can't be trusted not to hang).
Its not essential though, for sure.
Curiosity also plays the role here. :)
Though if you don't want, I can as well
not add a TODO comment to the code.
On Wed, 2023-06-21 at 15:42 +0500, stsp wrote:
> 21.06.2023 15:35, Jeff Layton пишет:
> > I don't think we can change this at this point.
> >
> > The bottom line (again) is that OFD locks are owned by the file
> > descriptor (much like with flock()), and since file descriptors can be
> > shared across multiple process it's impossible to say that some single
> > process owns it.
> What's the problem with 2 owners?
> Can't you get one of them, rather than
> meaningless -1?
> Compare this situation with read locks.
> They can overlap, so when you get an
> info about a read lock (except for the
> new F_UNLCK case), you get the info
> about *some* of the locks in that range.
> In the case of multiple owners, you
> likewise get the info about about some
> owner. If you iteratively send them a
> "please release this lock" message
> (eg in a form of SIGKILL), then you
> traverse all, and end up with the
> lock-free area.
> Is there really any problem here?
Yes. Ambiguous answers are worse than none at all.
What problem are you trying to solve by having F_OFD_GETLK report a pid?
--
Jeff Layton <[email protected]>
21.06.2023 16:22, stsp пишет:
>
> 21.06.2023 16:05, Jeff Layton пишет:
>> Yes. Ambiguous answers are worse than none at all.
>
> But same for read locks, when you
> query them with F_OFD_GETLK.
> It doesn't sound ambiguous to me,
> you get the valid owner, and you can
> iterate them if you kill them in a
> process (same as for read locks).
And in fact, no, you can't iterate the
read locks *unless* you use pid to
kill its owners! :) So this way you can
actually iterate the read locks, but
not in a most convenient way. :)
From: Matthew Wilcox
> Sent: 20 June 2023 14:46
>
> On Tue, Jun 20, 2023 at 06:39:07PM +0500, stsp wrote:
> > Though it will, for sure, represent the
> > task that _owns_ the lock.
>
> No, it *DOESN'T*. I can open a file, SCM_RIGHTS pass it to another task
> and then exit. Now the only owner of that lock is the recipient ...
> who may not even have received the fd yet.
Do these locks persist across fork+exec?
What happens is a completely unrelated process opens /proc/<pid>/fd
while a lock is held and then the (nominally) lock-holding
process closes the fd (or exits).
While it might be a useful diagnostic to know the pid of the
process that acquired the lock it clearly has no relationship
with any process that currently has an fd[] table entry that
references the file.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Wed, Jun 21, 2023 at 07:05:12AM -0400, Jeff Layton wrote:
> On Wed, 2023-06-21 at 15:42 +0500, stsp wrote:
> > 21.06.2023 15:35, Jeff Layton пишет:
> > > I don't think we can change this at this point.
> > >
> > > The bottom line (again) is that OFD locks are owned by the file
> > > descriptor (much like with flock()), and since file descriptors can be
> > > shared across multiple process it's impossible to say that some single
> > > process owns it.
> > What's the problem with 2 owners?
> > Can't you get one of them, rather than
> > meaningless -1?
> > Compare this situation with read locks.
> > They can overlap, so when you get an
> > info about a read lock (except for the
> > new F_UNLCK case), you get the info
> > about *some* of the locks in that range.
> > In the case of multiple owners, you
> > likewise get the info about about some
> > owner. If you iteratively send them a
> > "please release this lock" message
> > (eg in a form of SIGKILL), then you
> > traverse all, and end up with the
> > lock-free area.
> > Is there really any problem here?
>
> Yes. Ambiguous answers are worse than none at all.
I agree.
A few minor observations:
SCM_RIGHTS have already been mentioned multiple times. But I'm not sure
it's been mentioned explicitly but that trivially means it's possible to
send an fd to a completely separate thread-group, then kill off the
sending thread-group by killing their thread-group leader. Bad enough as
the identifier is now useless. But it also means that at some later
point that pid can be recycled. So using that pid for killing sprees is
usually a bad idea and might cause killing a completely separate
thread-group.
pidfd_getfd() can be used to retrieve a file descriptor from another
process; like an inverse dup. Opens up similar problems as SCM_RIGHTS.
Expressing ownership doesn't make sense as this is an inherently shared
concept as Jeff mentioned. So best case is to try and track the creator
similar to SO_PEERCRED for unix sockets.
In any case, this is all made worse by the fact that struct file_lock
stashes raw pid numbers. That means that the kernel itself can't even
detect whether the creator of that lock has died and the pid possibly
been recycled. In essence, it risk reporting as owner a random process
that just happens to have the same pid number as the creator that
might've died.
Even for unix sockets SO_PEERCRED and SCM_CREDS are inherently racy even
though they stash a struct pid instead of a raw pid number and that
makes them problematic in a lot of scenario which is why they will
support pidfds with v6.5.
The situation could probably be improved by stashing a struct pid in
struct file_lock then you could at least somewhat reasonably track the
creator as the kernel would be able to detect whether the creating
process has already been reaped (not exited). For example, I did this
for pidfds where it reports -1 if the process the pidfd refers to has
been reaped.
Btw, is there a way to limit the number of file locks a user might
create? Is that implicitly bound to the maximum number of fds and files
a caller may create because I saw RLIMIT_LOCKS but I didn't see it
checked anywhere.
23.06.2023 20:25, Christian Brauner пишет:
> On Wed, Jun 21, 2023 at 07:05:12AM -0400, Jeff Layton wrote:
>> On Wed, 2023-06-21 at 15:42 +0500, stsp wrote:
>>> 21.06.2023 15:35, Jeff Layton пишет:
>>>> I don't think we can change this at this point.
>>>>
>>>> The bottom line (again) is that OFD locks are owned by the file
>>>> descriptor (much like with flock()), and since file descriptors can be
>>>> shared across multiple process it's impossible to say that some single
>>>> process owns it.
>>> What's the problem with 2 owners?
>>> Can't you get one of them, rather than
>>> meaningless -1?
>>> Compare this situation with read locks.
>>> They can overlap, so when you get an
>>> info about a read lock (except for the
>>> new F_UNLCK case), you get the info
>>> about *some* of the locks in that range.
>>> In the case of multiple owners, you
>>> likewise get the info about about some
>>> owner. If you iteratively send them a
>>> "please release this lock" message
>>> (eg in a form of SIGKILL), then you
>>> traverse all, and end up with the
>>> lock-free area.
>>> Is there really any problem here?
>> Yes. Ambiguous answers are worse than none at all.
> I agree.
>
> A few minor observations:
>
> SCM_RIGHTS have already been mentioned multiple times. But I'm not sure
> it's been mentioned explicitly but that trivially means it's possible to
> send an fd to a completely separate thread-group, then kill off the
> sending thread-group by killing their thread-group leader. Bad enough as
> the identifier is now useless. But it also means that at some later
> point that pid can be recycled.
Come on.
I never proposed anything like this.
Of course the returned pid should be
the pid of the current, actual owner,
or one of current owners.
If someone else proposed to return
stalled pids, then it wasn't me.
On Fri, 2023-06-23 at 22:18 +0500, stsp wrote:
> 23.06.2023 20:25, Christian Brauner пишет:
> > On Wed, Jun 21, 2023 at 07:05:12AM -0400, Jeff Layton wrote:
> > > On Wed, 2023-06-21 at 15:42 +0500, stsp wrote:
> > > > 21.06.2023 15:35, Jeff Layton пишет:
> > > > > I don't think we can change this at this point.
> > > > >
> > > > > The bottom line (again) is that OFD locks are owned by the file
> > > > > descriptor (much like with flock()), and since file descriptors can be
> > > > > shared across multiple process it's impossible to say that some single
> > > > > process owns it.
> > > > What's the problem with 2 owners?
> > > > Can't you get one of them, rather than
> > > > meaningless -1?
> > > > Compare this situation with read locks.
> > > > They can overlap, so when you get an
> > > > info about a read lock (except for the
> > > > new F_UNLCK case), you get the info
> > > > about *some* of the locks in that range.
> > > > In the case of multiple owners, you
> > > > likewise get the info about about some
> > > > owner. If you iteratively send them a
> > > > "please release this lock" message
> > > > (eg in a form of SIGKILL), then you
> > > > traverse all, and end up with the
> > > > lock-free area.
> > > > Is there really any problem here?
> > > Yes. Ambiguous answers are worse than none at all.
> > I agree.
> >
> > A few minor observations:
> >
> > SCM_RIGHTS have already been mentioned multiple times. But I'm not sure
> > it's been mentioned explicitly but that trivially means it's possible to
> > send an fd to a completely separate thread-group, then kill off the
> > sending thread-group by killing their thread-group leader. Bad enough as
> > the identifier is now useless. But it also means that at some later
> > point that pid can be recycled.
> Come on.
> I never proposed anything like this.
> Of course the returned pid should be
> the pid of the current, actual owner,
> or one of current owners.
> If someone else proposed to return
> stalled pids, then it wasn't me.
Beyond all of this, there is a long history of problems with the l_pid
field as well with network filesystems, even with traditional POSIX
locks. What should go into the l_pid when a traditional POSIX lock is
held by a process on a separate host?
While POSIX mandates it, the l_pid is really sort of a "legacy" field
that is really just for informational purposes only nowadays. It might
have been a reliable bit of information back in the 1980's, but even
since the 90's it was suspect as a source of information.
Even if you _know_ you hold a traditional POSIX lock, be careful
trusting the information in that field.
--
Jeff Layton <[email protected]>
27.06.2023 21:00, Jeff Layton пишет:
> Beyond all of this, there is a long history of problems with the l_pid
> field as well with network filesystems, even with traditional POSIX
> locks. What should go into the l_pid when a traditional POSIX lock is
> held by a process on a separate host?
>
> While POSIX mandates it, the l_pid is really sort of a "legacy" field
> that is really just for informational purposes only nowadays. It might
> have been a reliable bit of information back in the 1980's, but even
> since the 90's it was suspect as a source of information.
>
> Even if you _know_ you hold a traditional POSIX lock, be careful
> trusting the information in that field.
Thanks for info.
Additional problem with multiple owners
that I can think of, is that you don't know
if more owners are present. And even if
you use SIGKILL to "iterate", you still don't
know if you got another owner of the prev
lock, or maybe you got entirely different
read lock with the same range from another
owner.
Still if you do "man fcntl" you'll see this:
pid_t l_pid; /* PID of process blocking our lock
(set by F_GETLK and F_OFD_GETLK) */
And no, its not my patch that did this. :)
So unless properly documented, this would
be treated as a bug. And it should _not_ be
documented as "OFD locks has no owner by
definition" or alike - no one buys that.