Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
in Unix socket's send and receive path) introduced a use-after-free bug.
It passes the scm reference to the first skb. Skb(s) afterwards may
reference freed data structure because the first skb can be destructed
by the receiver at anytime. The fix is by passing the scm reference to
the very last skb.
Signed-off-by: Zheng Yan <redacted>
Reported-by: Jiri Slaby <redacted>
---
@@ -1637,12 +1638,15 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,*/size=min_t(int,size,skb_tailroom(skb));+/* pass the scm reference to the very last skb */+if(sent+size>=len)+scm_ref=false;-/* Only send the fds and no ref to pid in the first buffer */-err=unix_scm_to_skb(siocb->scm,skb,!fds_sent,fds_sent);+/* Only send the fds in the first buffer */+err=unix_scm_to_skb(siocb->scm,skb,!fds_sent,scm_ref);if(err<0){kfree_skb(skb);-gotoout;+gotoout_err;}max_level=err+1;fds_sent=true;
On Sun, Sep 4, 2011 at 7:44 AM, Yan, Zheng [off-list ref] wrote:
Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
in Unix socket's send and receive path) introduced a use-after-free bug.
It passes the scm reference to the first skb. Skb(s) afterwards may
reference freed data structure because the first skb can be destructed
by the receiver at anytime. The fix is by passing the scm reference to
the very last skb.
s/by passing/bypassing ?
Signed-off-by: Zheng Yan <redacted>
Reported-by: Jiri Slaby <redacted>
---
Tested on i386 against linux-next (next-20110831).
- Sedat -
On Sun, Sep 4, 2011 at 3:12 PM, Sedat Dilek [off-list ref] wrote:
On Sun, Sep 4, 2011 at 7:44 AM, Yan, Zheng [off-list ref] wrote:
quoted
Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
in Unix socket's send and receive path) introduced a use-after-free bug.
It passes the scm reference to the first skb. Skb(s) afterwards may
reference freed data structure because the first skb can be destructed
by the receiver at anytime. The fix is by passing the scm reference to
the very last skb.
s/by passing/bypassing ?
No
quoted
Signed-off-by: Zheng Yan <redacted>
Reported-by: Jiri Slaby <redacted>
---
Tested on i386 against linux-next (next-20110831).
Thank you.
- Sedat -
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Joe Perches <joe@perches.com> Date: 2011-09-04 15:50:12
On Sun, 2011-09-04 at 16:23 +0800, Yan, Zheng wrote:
On Sun, Sep 4, 2011 at 3:12 PM, Sedat Dilek [off-list ref] wrote:
quoted
On Sun, Sep 4, 2011 at 7:44 AM, Yan, Zheng [off-list ref] wrote:
quoted
It passes the scm reference to the first skb. Skb(s) afterwards may
reference freed data structure because the first skb can be destructed
by the receiver at anytime. The fix is by passing the scm reference to
the very last skb.
s/by passing/bypassing ?
No
(putting on my Randy Dunlap hat)
The issue was fixed by passing...
or maybe
The fix is to pass...
On Sun, 2011-09-04 at 13:44 +0800, Yan, Zheng wrote:
Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
in Unix socket's send and receive path) introduced a use-after-free bug.
It passes the scm reference to the first skb. Skb(s) afterwards may
reference freed data structure because the first skb can be destructed
by the receiver at anytime. The fix is by passing the scm reference to
the very last skb.
Signed-off-by: Zheng Yan <redacted>
Reported-by: Jiri Slaby <redacted>
---
Thanks for finding this bug in my original patch. I've missed the case
where receiving side could have released the all the references to the
credential before the send side is using the credential again for
subsequent skbs in the stream, thus causing the problem we saw. Getting
an extra reference for pid/credentials at the beginning of the stream
and not getting reference for the last skb is the right approach.
Thanks also to Sedat, Valdis and Jiri for their extensive testing to
discover the bug and testing the subsequent fixes.
Acked-by: Tim Chen <redacted>
On Sun, 2011-09-04 at 16:23 +0800, Yan, Zheng wrote:
On Sun, Sep 4, 2011 at 3:12 PM, Sedat Dilek [off-list ref] wrote:
quoted
On Sun, Sep 4, 2011 at 7:44 AM, Yan, Zheng [off-list ref] wrote:
quoted
Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
in Unix socket's send and receive path) introduced a use-after-free bug.
It passes the scm reference to the first skb. Skb(s) afterwards may
reference freed data structure because the first skb can be destructed
by the receiver at anytime. The fix is by passing the scm reference to
the very last skb.
s/by passing/bypassing ?
No
Maybe it is a clearer to say
The fix is by withholding the scm reference obtained at the beginning of
unix_stream_sendmsg via scm_send and pass it to the very last skb.
Tim
From: Eric Dumazet <hidden> Date: 2011-09-06 17:40:06
Le mardi 06 septembre 2011 à 09:25 -0700, Tim Chen a écrit :
On Sun, 2011-09-04 at 13:44 +0800, Yan, Zheng wrote:
quoted
Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
in Unix socket's send and receive path) introduced a use-after-free bug.
It passes the scm reference to the first skb. Skb(s) afterwards may
reference freed data structure because the first skb can be destructed
by the receiver at anytime. The fix is by passing the scm reference to
the very last skb.
Signed-off-by: Zheng Yan <redacted>
Reported-by: Jiri Slaby <redacted>
---
Thanks for finding this bug in my original patch. I've missed the case
where receiving side could have released the all the references to the
credential before the send side is using the credential again for
subsequent skbs in the stream, thus causing the problem we saw. Getting
an extra reference for pid/credentials at the beginning of the stream
and not getting reference for the last skb is the right approach.
Thanks also to Sedat, Valdis and Jiri for their extensive testing to
discover the bug and testing the subsequent fixes.
Acked-by: Tim Chen <redacted>
What happens if message must be split in two skb,
first skb is built, queued (without scm reference)
Second skb allocation fails.
Rule about refs/norefs games is : As soon as you put skb into a list, it
should have all appropriate references if this skb has pointer(s) to
objects(s)
We should revert 0856a304091b33a and code the thing differently.
Instead of storing pointer to pid and cred in UNIXSKB(), why dont we
copy all needed information ? No ref counts at all.
skb->cb[] is large enough.
On Tue, 2011-09-06 at 19:40 +0200, Eric Dumazet wrote:
Le mardi 06 septembre 2011 à 09:25 -0700, Tim Chen a écrit :
quoted
On Sun, 2011-09-04 at 13:44 +0800, Yan, Zheng wrote:
quoted
Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
in Unix socket's send and receive path) introduced a use-after-free bug.
It passes the scm reference to the first skb. Skb(s) afterwards may
reference freed data structure because the first skb can be destructed
by the receiver at anytime. The fix is by passing the scm reference to
the very last skb.
Signed-off-by: Zheng Yan <redacted>
Reported-by: Jiri Slaby <redacted>
---
Thanks for finding this bug in my original patch. I've missed the case
where receiving side could have released the all the references to the
credential before the send side is using the credential again for
subsequent skbs in the stream, thus causing the problem we saw. Getting
an extra reference for pid/credentials at the beginning of the stream
and not getting reference for the last skb is the right approach.
Thanks also to Sedat, Valdis and Jiri for their extensive testing to
discover the bug and testing the subsequent fixes.
Acked-by: Tim Chen <redacted>
What happens if message must be split in two skb,
first skb is built, queued (without scm reference)
An extra scm reference is already first obtained in scm_send at the
beginning of unix_stream_sendmsg in Yan Zheng's patch. So things should
be okay as long as we only use this extra reference we got in scm_send
for the last skb in unix_stream_sendmsg instead of the first skb.
Second skb allocation fails.
Rule about refs/norefs games is : As soon as you put skb into a list, it
should have all appropriate references if this skb has pointer(s) to
objects(s)
All the skbs put on the list does have proper reference on pid/scm. In
the example you give, the first skb got the reference at this line:
err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
the second skb use the reference already obtained at the beginning of
unix_stream_sendmsg if the skb allocation is successful:
err = scm_send(sock, msg, siocb->scm);
Now if the second skb allocation failed, the extra scm reference will be
released by scm_destroy in the error handling path.
We should revert 0856a304091b33a and code the thing differently.
Instead of storing pointer to pid and cred in UNIXSKB(), why dont we
copy all needed information ? No ref counts at all.
skb->cb[] is large enough.
If we can simply copy some information over, that will be ideal and
will resolve all the scalability problems.
However, I don't see other obvious info that we can pass to avoid
passing pid. Our current credential is pid and uid based, and requires
the knowledge of sender's pid to interpret uid to do credentials
checking. So without passing the sender pid, I don't see an easy way
for the receive side to interpret sender uid it got, which is needed in
user_ns_map_uid function when we call cred_to_ucred.
I was trying to do minimal changes to gain some performance. The
approach you suggest is great but will probably require much more
changes to the credentials infrastructure. Or maybe there are some easy
way to do it that I don't see.
Thanks.
Tim
From: Eric Dumazet <hidden> Date: 2011-09-06 19:01:11
Le mardi 06 septembre 2011 à 11:50 -0700, Tim Chen a écrit :
On Tue, 2011-09-06 at 19:40 +0200, Eric Dumazet wrote:
quoted
Le mardi 06 septembre 2011 à 09:25 -0700, Tim Chen a écrit :
quoted
On Sun, 2011-09-04 at 13:44 +0800, Yan, Zheng wrote:
quoted
Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
in Unix socket's send and receive path) introduced a use-after-free bug.
It passes the scm reference to the first skb. Skb(s) afterwards may
reference freed data structure because the first skb can be destructed
by the receiver at anytime. The fix is by passing the scm reference to
the very last skb.
Signed-off-by: Zheng Yan <redacted>
Reported-by: Jiri Slaby <redacted>
---
Thanks for finding this bug in my original patch. I've missed the case
where receiving side could have released the all the references to the
credential before the send side is using the credential again for
subsequent skbs in the stream, thus causing the problem we saw. Getting
an extra reference for pid/credentials at the beginning of the stream
and not getting reference for the last skb is the right approach.
Thanks also to Sedat, Valdis and Jiri for their extensive testing to
discover the bug and testing the subsequent fixes.
Acked-by: Tim Chen <redacted>
What happens if message must be split in two skb,
first skb is built, queued (without scm reference)
An extra scm reference is already first obtained in scm_send at the
beginning of unix_stream_sendmsg in Yan Zheng's patch. So things should
be okay as long as we only use this extra reference we got in scm_send
for the last skb in unix_stream_sendmsg instead of the first skb.
quoted
Second skb allocation fails.
Rule about refs/norefs games is : As soon as you put skb into a list, it
should have all appropriate references if this skb has pointer(s) to
objects(s)
All the skbs put on the list does have proper reference on pid/scm. In
the example you give, the first skb got the reference at this line:
err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
This is the current code. We know its buggy.
I was discussing of things after proposed patch, not current net-next.
This reads :
err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, scm_ref);
So first skb is sent without ref taken, as mentioned in Changelog ?
If second skb cannot be built, we exit this system call with an already
queued skb. Receiver can then access to freed memory.
the second skb use the reference already obtained at the beginning of
unix_stream_sendmsg if the skb allocation is successful:
err = scm_send(sock, msg, siocb->scm);
Now if the second skb allocation failed, the extra scm reference will be
released by scm_destroy in the error handling path.
quoted
We should revert 0856a304091b33a and code the thing differently.
Instead of storing pointer to pid and cred in UNIXSKB(), why dont we
copy all needed information ? No ref counts at all.
skb->cb[] is large enough.
If we can simply copy some information over, that will be ideal and
will resolve all the scalability problems.
However, I don't see other obvious info that we can pass to avoid
passing pid. Our current credential is pid and uid based, and requires
the knowledge of sender's pid to interpret uid to do credentials
checking. So without passing the sender pid, I don't see an easy way
for the receive side to interpret sender uid it got, which is needed in
user_ns_map_uid function when we call cred_to_ucred.
I was trying to do minimal changes to gain some performance. The
approach you suggest is great but will probably require much more
changes to the credentials infrastructure. Or maybe there are some easy
way to do it that I don't see.
My approach would basically revert the 7361c36c commit too :(
I am sorry, but the only way to avoid too many pid/cred references is to
lock the socket [aka unix_state_lock(other);] for the whole send()
duration.
This way, you can really increment the pid/cred reference on the last
pushed skb, because no reader can 'catch first skb'
As soon as unix_state_unlock(other) is called, everything can happen, so
skb must be self contained, as I stated in my earlier mail.
On Tue, 2011-09-06 at 21:01 +0200, Eric Dumazet wrote:
quoted
All the skbs put on the list does have proper reference on pid/scm. In
the example you give, the first skb got the reference at this line:
err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
This is the current code. We know its buggy.
I was discussing of things after proposed patch, not current net-next.
I think we are on the same page.
My approach would basically revert the 7361c36c commit too :(
I think so. I was not fond of commit 7361c36c as it caused a 90%
regression in threaded case of hackbench that we noticed back in 2.6.36
days. If there's some way to undo its evil, I'm all for it.
I am sorry, but the only way to avoid too many pid/cred references is to
lock the socket [aka unix_state_lock(other);] for the whole send()
duration.
Yes, I think locking the sendmsg for the entire duration of
unix_stream_sendmsg makes a lot of sense. It simplifies the logic a lot
more. I'll try to cook something up in the next couple of days.
This way, you can really increment the pid/cred reference on the last
pushed skb, because no reader can 'catch first skb'
As soon as unix_state_unlock(other) is called, everything can happen, so
skb must be self contained, as I stated in my earlier mail.
From: Eric Dumazet <hidden> Date: 2011-09-06 19:43:34
Le mardi 06 septembre 2011 à 12:33 -0700, Tim Chen a écrit :
Yes, I think locking the sendmsg for the entire duration of
unix_stream_sendmsg makes a lot of sense. It simplifies the logic a lot
more. I'll try to cook something up in the next couple of days.
Thats not really possible, we cant hold a spinlock and call
sock_alloc_send_skb() and/or memcpy_fromiovec(), wich might sleep.
You would need to prepare the full skb list, then :
- stick the ref on the last skb of the list.
Transfert the whole skb list in other->sk_receive_queue in one go,
instead of one after another.
Unfortunately, this would break streaming (big send(), and another
thread doing the receive)
Listen, I am wondering why hackbench even triggers SCM code. This is
really odd. We should not have a _single_ pid/cred ref/unref at all.
On Tue, 2011-09-06 at 21:43 +0200, Eric Dumazet wrote:
Le mardi 06 septembre 2011 à 12:33 -0700, Tim Chen a écrit :
quoted
Yes, I think locking the sendmsg for the entire duration of
unix_stream_sendmsg makes a lot of sense. It simplifies the logic a lot
more. I'll try to cook something up in the next couple of days.
Thats not really possible, we cant hold a spinlock and call
sock_alloc_send_skb() and/or memcpy_fromiovec(), wich might sleep.
You would need to prepare the full skb list, then :
- stick the ref on the last skb of the list.
Transfert the whole skb list in other->sk_receive_queue in one go,
instead of one after another.
Unfortunately, this would break streaming (big send(), and another
thread doing the receive)
Listen, I am wondering why hackbench even triggers SCM code. This is
really odd. We should not have a _single_ pid/cred ref/unref at all.
Hackbench triggers the code because it has a bunch of threads sending
msgs on UNIX socket.
Well, if the lock socket approach doesn't work, then my original patch
plus Yan Zheng's fix should still work. I'll try to answer your
objections below:
I was discussing of things after proposed patch, not current net-next.
This reads :
err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, scm_ref);
So first skb is sent without ref taken, as mentioned in Changelog ?
No. the first skb is sent *with* ref taken, as scm_ref is set to true for
first skb.
If second skb cannot be built, we exit this system call with an already
queued skb. Receiver can then access to freed memory.
No, we do have reference set. For first skb, in unix_scm_to_skb. For the
second skb (which is the last skb), in scm_sent. Should the second skb alloc failed,
we'll release the ref in scm_destroy. Otherwise, the receiver will release
the references will consuming the skb.
Tim
From: Eric Dumazet <hidden> Date: 2011-09-06 20:20:00
Le mardi 06 septembre 2011 à 12:59 -0700, Tim Chen a écrit :
On Tue, 2011-09-06 at 21:43 +0200, Eric Dumazet wrote:
quoted
Le mardi 06 septembre 2011 à 12:33 -0700, Tim Chen a écrit :
quoted
Yes, I think locking the sendmsg for the entire duration of
unix_stream_sendmsg makes a lot of sense. It simplifies the logic a lot
more. I'll try to cook something up in the next couple of days.
Thats not really possible, we cant hold a spinlock and call
sock_alloc_send_skb() and/or memcpy_fromiovec(), wich might sleep.
You would need to prepare the full skb list, then :
- stick the ref on the last skb of the list.
Transfert the whole skb list in other->sk_receive_queue in one go,
instead of one after another.
Unfortunately, this would break streaming (big send(), and another
thread doing the receive)
Listen, I am wondering why hackbench even triggers SCM code. This is
really odd. We should not have a _single_ pid/cred ref/unref at all.
Hackbench triggers the code because it has a bunch of threads sending
msgs on UNIX socket.
quoted
Well, if the lock socket approach doesn't work, then my original patch
plus Yan Zheng's fix should still work. I'll try to answer your
objections below:
quoted
I was discussing of things after proposed patch, not current net-next.
This reads :
err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, scm_ref);
So first skb is sent without ref taken, as mentioned in Changelog ?
No. the first skb is sent *with* ref taken, as scm_ref is set to true for
first skb.
quoted
If second skb cannot be built, we exit this system call with an already
queued skb. Receiver can then access to freed memory.
No, we do have reference set. For first skb, in unix_scm_to_skb. For the
second skb (which is the last skb), in scm_sent. Should the second skb alloc failed,
we'll release the ref in scm_destroy. Otherwise, the receiver will release
the references will consuming the skb.
This is crap. This is not the intent of the code I read from the patch.
unless scm_ref really means scm_noref ?
I really hate this patch. I mean it.
I read it 10 times, spent 2 hours and still dont understand it.
@@ -1577,6 +1577,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock, int sent = 0; struct scm_cookie tmp_scm; bool fds_sent = false;+ bool scm_ref = true; int max_level; if (NULL == siocb->scm)
@@ -1637,12 +1638,15 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock, */ size = min_t(int, size, skb_tailroom(skb));+ /* pass the scm reference to the very last skb */
HERE: I understand : on the last skb, set scm_ref to false.
So comment is wrong.
+ if (sent + size >= len)
+ scm_ref = false;
- /* Only send the fds and no ref to pid in the first buffer */
- err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
+ /* Only send the fds in the first buffer */
+ err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, scm_ref);
if (err < 0) {
kfree_skb(skb);
- goto out;
+ goto out_err;
}
As I said, we should revert the buggy patch, and rewrite a performance
fix from scratch, with not a single get_pid()/put_pid() in fast path.
read()/write() on AF_UNIX sockets should not use a single
get_pid()/put_pid().
This is a serious regression we should fix at 100%, not 50% or even 75%,
adding serious bugs.
On Tue, 2011-09-06 at 22:19 +0200, Eric Dumazet wrote:
unless scm_ref really means scm_noref ?
I really hate this patch. I mean it.
I read it 10 times, spent 2 hours and still dont understand it.
Eric,
I've tried another patch to fix my original one. I've used a boolean
ref_avail to indicate if there is an outstanding ref to scm not yet
encoded into the skb. Hopefully the logic is clearer in this new patch.
As I said, we should revert the buggy patch, and rewrite a performance
fix from scratch, with not a single get_pid()/put_pid() in fast path.
read()/write() on AF_UNIX sockets should not use a single
get_pid()/put_pid().
This is a serious regression we should fix at 100%, not 50% or even 75%,
adding serious bugs.
That will be ideal if there is another way to fix it 100%, other than reverting
commit 7361c36c. Probably if there is some way we know beforehand that
both sender and receiver share the same pid, which is quite common, a
lot of these pid code can be bypassed.
Tim
Signed-off-by: Tim Chen <redacted>
---
@@ -1582,11 +1582,13 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,structscm_cookietmp_scm;boolfds_sent=false;intmax_level;+boolref_avail;/* scm ref not yet used in skb */if(NULL==siocb->scm)siocb->scm=&tmp_scm;wait_for_unix_gc();err=scm_send(sock,msg,siocb->scm);+ref_avail=true;if(err<0)returnerr;
@@ -1642,11 +1644,18 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,size=min_t(int,size,skb_tailroom(skb));-/* Only send the fds and no ref to pid in the first buffer */-err=unix_scm_to_skb(siocb->scm,skb,!fds_sent,fds_sent);+/* encode scm in skb and use the scm ref */+ref_avail=false;+if(sent+size<len){+/* Only send the fds in the first buffer */+/* get additional ref if more skbs will be created */+err=unix_scm_to_skb(siocb->scm,skb,!fds_sent,true);+ref_avail=true;+}else+err=unix_scm_to_skb(siocb->scm,skb,!fds_sent,false);if(err<0){kfree_skb(skb);-gotoout;+gotoout_err;}max_level=err+1;fds_sent=true;
On Wed, Sep 7, 2011 at 4:19 AM, Eric Dumazet [off-list ref] wrote:
Le mardi 06 septembre 2011 à 12:59 -0700, Tim Chen a écrit :
quoted
On Tue, 2011-09-06 at 21:43 +0200, Eric Dumazet wrote:
quoted
Le mardi 06 septembre 2011 à 12:33 -0700, Tim Chen a écrit :
quoted
Yes, I think locking the sendmsg for the entire duration of
unix_stream_sendmsg makes a lot of sense. It simplifies the logic a lot
more. I'll try to cook something up in the next couple of days.
Thats not really possible, we cant hold a spinlock and call
sock_alloc_send_skb() and/or memcpy_fromiovec(), wich might sleep.
You would need to prepare the full skb list, then :
- stick the ref on the last skb of the list.
Transfert the whole skb list in other->sk_receive_queue in one go,
instead of one after another.
Unfortunately, this would break streaming (big send(), and another
thread doing the receive)
Listen, I am wondering why hackbench even triggers SCM code. This is
really odd. We should not have a _single_ pid/cred ref/unref at all.
Hackbench triggers the code because it has a bunch of threads sending
msgs on UNIX socket.
quoted
Well, if the lock socket approach doesn't work, then my original patch
plus Yan Zheng's fix should still work. I'll try to answer your
objections below:
quoted
I was discussing of things after proposed patch, not current net-next.
This reads :
err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, scm_ref);
So first skb is sent without ref taken, as mentioned in Changelog ?
No. the first skb is sent *with* ref taken, as scm_ref is set to true for
first skb.
quoted
If second skb cannot be built, we exit this system call with an already
queued skb. Receiver can then access to freed memory.
No, we do have reference set. For first skb, in unix_scm_to_skb. For the
second skb (which is the last skb), in scm_sent. Should the second skb alloc failed,
we'll release the ref in scm_destroy. Otherwise, the receiver will release
the references will consuming the skb.
This is crap. This is not the intent of the code I read from the patch.
unless scm_ref really means scm_noref ?
I really hate this patch. I mean it.
I read it 10 times, spent 2 hours and still dont understand it.
Sorry, scm_ref means "sender hold a scm reference". I should add comment for it.
*/
size = min_t(int, size, skb_tailroom(skb));
+ /* pass the scm reference to the very last skb */
HERE: I understand : on the last skb, set scm_ref to false.
So comment is wrong.
+ if (sent + size >= len)
+ scm_ref = false;
- /* Only send the fds and no ref to pid in the first buffer */
- err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
+ /* Only send the fds in the first buffer */
+ err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, scm_ref);
if (err < 0) {
kfree_skb(skb);
- goto out;
+ goto out_err;
}
As I said, we should revert the buggy patch, and rewrite a performance
fix from scratch, with not a single get_pid()/put_pid() in fast path.
read()/write() on AF_UNIX sockets should not use a single
get_pid()/put_pid().
This is a serious regression we should fix at 100%, not 50% or even 75%,
adding serious bugs.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <hidden> Date: 2011-09-07 02:35:22
Le mardi 06 septembre 2011 à 15:08 -0700, Tim Chen a écrit :
On Tue, 2011-09-06 at 22:19 +0200, Eric Dumazet wrote:
quoted
unless scm_ref really means scm_noref ?
I really hate this patch. I mean it.
I read it 10 times, spent 2 hours and still dont understand it.
Eric,
I've tried another patch to fix my original one. I've used a boolean
ref_avail to indicate if there is an outstanding ref to scm not yet
encoded into the skb. Hopefully the logic is clearer in this new patch.
quoted
As I said, we should revert the buggy patch, and rewrite a performance
fix from scratch, with not a single get_pid()/put_pid() in fast path.
read()/write() on AF_UNIX sockets should not use a single
get_pid()/put_pid().
This is a serious regression we should fix at 100%, not 50% or even 75%,
adding serious bugs.
That will be ideal if there is another way to fix it 100%, other than reverting
commit 7361c36c. Probably if there is some way we know beforehand that
both sender and receiver share the same pid, which is quite common, a
lot of these pid code can be bypassed.
Let me restate : Its should be obvious to fix the performance hit for
good.
If namespaces are not used (CONFIG_PID_NS is not set), we can use the
old code, prior to commit 7361c36c : store pid/uid/gid in skb->cb[]
But more generally, when a write() is done on AF_UNIX socket, we pass a
NULL siocb->scm to unix_{dgram|stream}_sendmsg()
if (NULL == siocb->scm)
siocb->scm = &tmp_scm;
There is no need in this case to copy in each skb->cb, pointers to
struct pid and struct cred with their atomic reference being changed in
the sender and receiver.
We try to remove _all_ atomic ops on refcounts not only because atomic
ops are expensive by themselves, but also because of the cache line ping
pongs.
Tim
Signed-off-by: Tim Chen <redacted>
---
When a patch is wrong, you can admit it and ask for a revert, instead of
obfuscating the code so much that even a netdev guy like me doesnt
understand it anymore.
We speak of a very recent patch in net-next, not yet published to Linus
tree. There is no shame to revert it right now and work on a new patch.
I want to be able to track future bugs on this code, and your patch and
their fixes made functions too hard to read.
If you dont want to work on it, I'll do it myself.
From: Eric Dumazet <hidden> Date: 2011-09-07 02:55:39
Le mercredi 07 septembre 2011 à 07:09 +0800, Yan, Zheng a écrit :
On Wed, Sep 7, 2011 at 4:19 AM, Eric Dumazet [off-list ref] wrote:
quoted
Le mardi 06 septembre 2011 à 12:59 -0700, Tim Chen a écrit :
quoted
On Tue, 2011-09-06 at 21:43 +0200, Eric Dumazet wrote:
quoted
Le mardi 06 septembre 2011 à 12:33 -0700, Tim Chen a écrit :
quoted
Yes, I think locking the sendmsg for the entire duration of
unix_stream_sendmsg makes a lot of sense. It simplifies the logic a lot
more. I'll try to cook something up in the next couple of days.
Thats not really possible, we cant hold a spinlock and call
sock_alloc_send_skb() and/or memcpy_fromiovec(), wich might sleep.
You would need to prepare the full skb list, then :
- stick the ref on the last skb of the list.
Transfert the whole skb list in other->sk_receive_queue in one go,
instead of one after another.
Unfortunately, this would break streaming (big send(), and another
thread doing the receive)
Listen, I am wondering why hackbench even triggers SCM code. This is
really odd. We should not have a _single_ pid/cred ref/unref at all.
Hackbench triggers the code because it has a bunch of threads sending
msgs on UNIX socket.
quoted
Well, if the lock socket approach doesn't work, then my original patch
plus Yan Zheng's fix should still work. I'll try to answer your
objections below:
quoted
I was discussing of things after proposed patch, not current net-next.
This reads :
err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, scm_ref);
So first skb is sent without ref taken, as mentioned in Changelog ?
No. the first skb is sent *with* ref taken, as scm_ref is set to true for
first skb.
quoted
If second skb cannot be built, we exit this system call with an already
queued skb. Receiver can then access to freed memory.
No, we do have reference set. For first skb, in unix_scm_to_skb. For the
second skb (which is the last skb), in scm_sent. Should the second skb alloc failed,
we'll release the ref in scm_destroy. Otherwise, the receiver will release
the references will consuming the skb.
This is crap. This is not the intent of the code I read from the patch.
unless scm_ref really means scm_noref ?
I really hate this patch. I mean it.
I read it 10 times, spent 2 hours and still dont understand it.
Sorry, scm_ref means "sender hold a scm reference". I should add comment for it.
There is no "sender holds a scm reference" requirement.
The process is running, so holds a pid and cred by itself.
If pid/cred pointers are stuffed into skb->cb[], then each last skb must
holds its own reference to pid and cred.
Problem is : we dont know wich skb _is_ the last one, because we can
fail skb allocation or user->kernel copy any time.
Please David just revert 0856a304091b33a8e
I'll work today on a fix to performance regression added in 7361c36c
On Wed, Sep 7, 2011 at 4:19 AM, Eric Dumazet [off-list ref] wrote:
quoted hunk
Le mardi 06 septembre 2011 à 12:59 -0700, Tim Chen a écrit :
quoted
On Tue, 2011-09-06 at 21:43 +0200, Eric Dumazet wrote:
quoted
Le mardi 06 septembre 2011 à 12:33 -0700, Tim Chen a écrit :
quoted
Yes, I think locking the sendmsg for the entire duration of
unix_stream_sendmsg makes a lot of sense. It simplifies the logic a lot
more. I'll try to cook something up in the next couple of days.
Thats not really possible, we cant hold a spinlock and call
sock_alloc_send_skb() and/or memcpy_fromiovec(), wich might sleep.
You would need to prepare the full skb list, then :
- stick the ref on the last skb of the list.
Transfert the whole skb list in other->sk_receive_queue in one go,
instead of one after another.
Unfortunately, this would break streaming (big send(), and another
thread doing the receive)
Listen, I am wondering why hackbench even triggers SCM code. This is
really odd. We should not have a _single_ pid/cred ref/unref at all.
Hackbench triggers the code because it has a bunch of threads sending
msgs on UNIX socket.
quoted
Well, if the lock socket approach doesn't work, then my original patch
plus Yan Zheng's fix should still work. I'll try to answer your
objections below:
quoted
I was discussing of things after proposed patch, not current net-next.
This reads :
err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, scm_ref);
So first skb is sent without ref taken, as mentioned in Changelog ?
No. the first skb is sent *with* ref taken, as scm_ref is set to true for
first skb.
quoted
If second skb cannot be built, we exit this system call with an already
queued skb. Receiver can then access to freed memory.
No, we do have reference set. For first skb, in unix_scm_to_skb. For the
second skb (which is the last skb), in scm_sent. Should the second skb alloc failed,
we'll release the ref in scm_destroy. Otherwise, the receiver will release
the references will consuming the skb.
This is crap. This is not the intent of the code I read from the patch.
unless scm_ref really means scm_noref ?
I really hate this patch. I mean it.
I read it 10 times, spent 2 hours and still dont understand it.
*/
size = min_t(int, size, skb_tailroom(skb));
+ /* pass the scm reference to the very last skb */
HERE: I understand : on the last skb, set scm_ref to false.
So comment is wrong.
I guess you misunderstood this code. Set scm_ref to false means skb will inherit
sender's reference. Then we call unix_scm_to_skb() with parameter
'ref' == false.
So it doesn't get additional reference. I admit my patch is confusing,
but I think
Tim's new patch is OK. (even in the case of fail skb allocation or
user->kernel copy)
Regards
+ if (sent + size >= len)
+ scm_ref = false;
- /* Only send the fds and no ref to pid in the first buffer */
- err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
+ /* Only send the fds in the first buffer */
+ err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, scm_ref);
if (err < 0) {
kfree_skb(skb);
- goto out;
+ goto out_err;
}
As I said, we should revert the buggy patch, and rewrite a performance
fix from scratch, with not a single get_pid()/put_pid() in fast path.
read()/write() on AF_UNIX sockets should not use a single
get_pid()/put_pid().
This is a serious regression we should fix at 100%, not 50% or even 75%,
adding serious bugs.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <hidden> Date: 2011-09-07 05:08:34
Le mercredi 07 septembre 2011 à 12:36 +0800, Yan, Zheng a écrit :
I guess you misunderstood this code. Set scm_ref to false means skb will inherit
sender's reference. Then we call unix_scm_to_skb() with parameter
'ref' == false.
So it doesn't get additional reference. I admit my patch is confusing,
but I think
Tim's new patch is OK. (even in the case of fail skb allocation or
user->kernel copy)
I want to be able to review the code now, and in two or three years too,
without spending hours and hours.
Could you _please_ guys send a patch, with :
1) A good changelog : In this confusing area, this is probably more
important than actual code.
2) right logic and right variable names
I am sorry, but this is not good :
+ /* encode scm in skb and use the scm ref */
+ ref_avail = false;
+ if (sent + size < len) {
+ /* Only send the fds in the first buffer */
+ /* get additional ref if more skbs will be created */
+ err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, true);
+ ref_avail = true;
+ } else
+ err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, false);
Le mercredi 07 septembre 2011 à 12:36 +0800, Yan, Zheng a écrit :
quoted
I guess you misunderstood this code. Set scm_ref to false means skb will inherit
sender's reference. Then we call unix_scm_to_skb() with parameter
'ref' == false.
So it doesn't get additional reference. I admit my patch is confusing,
but I think
Tim's new patch is OK. (even in the case of fail skb allocation or
user->kernel copy)
I want to be able to review the code now, and in two or three years too,
without spending hours and hours.
Could you _please_ guys send a patch, with :
1) A good changelog : In this confusing area, this is probably more
important than actual code.
Sorry for my poor English.
2) right logic and right variable names
I am sorry, but this is not good :
+ /* encode scm in skb and use the scm ref */
+ ref_avail = false;
+ if (sent + size < len) {
+ /* Only send the fds in the first buffer */
+ /* get additional ref if more skbs will be created */
+ err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, true);
+ ref_avail = true;
+ } else
+ err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, false);
Is code like this OK? Thanks
---
if (sent + size < len) {
/* Only send the fds in the first buffer */
/* get additional ref if more skbs will be created */
err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, true);
} else {
err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, false);
ref_avail = false;
}
On Tue, Sep 6, 2011 at 9:59 PM, Tim Chen [off-list ref] wrote:
On Tue, 2011-09-06 at 21:43 +0200, Eric Dumazet wrote:
quoted
Le mardi 06 septembre 2011 à 12:33 -0700, Tim Chen a écrit :
quoted
Yes, I think locking the sendmsg for the entire duration of
unix_stream_sendmsg makes a lot of sense. It simplifies the logic a lot
more. I'll try to cook something up in the next couple of days.
Thats not really possible, we cant hold a spinlock and call
sock_alloc_send_skb() and/or memcpy_fromiovec(), wich might sleep.
You would need to prepare the full skb list, then :
- stick the ref on the last skb of the list.
Transfert the whole skb list in other->sk_receive_queue in one go,
instead of one after another.
Unfortunately, this would break streaming (big send(), and another
thread doing the receive)
Listen, I am wondering why hackbench even triggers SCM code. This is
really odd. We should not have a _single_ pid/cred ref/unref at all.
Hackbench triggers the code because it has a bunch of threads sending
msgs on UNIX socket.
quoted
# lsof | grep socket | wc -l
198
Aprrox 200 sockets in usage here, can you post your hackbench line, please?
I would compare hackbench results with and without new improvements in SCM code.
- Sedat -
[...]
On Thu, 2011-09-08 at 12:05 +0200, Sedat Dilek wrote:
On Tue, Sep 6, 2011 at 9:59 PM, Tim Chen [off-list ref] wrote:
quoted
On Tue, 2011-09-06 at 21:43 +0200, Eric Dumazet wrote:
quoted
Le mardi 06 septembre 2011 à 12:33 -0700, Tim Chen a écrit :
quoted
Yes, I think locking the sendmsg for the entire duration of
unix_stream_sendmsg makes a lot of sense. It simplifies the logic a lot
more. I'll try to cook something up in the next couple of days.
Thats not really possible, we cant hold a spinlock and call
sock_alloc_send_skb() and/or memcpy_fromiovec(), wich might sleep.
You would need to prepare the full skb list, then :
- stick the ref on the last skb of the list.
Transfert the whole skb list in other->sk_receive_queue in one go,
instead of one after another.
Unfortunately, this would break streaming (big send(), and another
thread doing the receive)
Listen, I am wondering why hackbench even triggers SCM code. This is
really odd. We should not have a _single_ pid/cred ref/unref at all.
Hackbench triggers the code because it has a bunch of threads sending
msgs on UNIX socket.
quoted
# lsof | grep socket | wc -l
198
Aprrox 200 sockets in usage here, can you post your hackbench line, please?
I would compare hackbench results with and without new improvements in SCM code.
- Sedat -
The hackbench line I used was
./hackbench 50 thread 2000
You will need to use the threaded case for testing to see the issue. I
was running on a 4 socket, 40 cores total Westmere-EX machine. The
improvement may depend on your machine size, probably with more
improvement on larger multi-socket machine as smaller ones don't have as
big a problem.
Tim
Eric,
Can you re-spin a patch that incorporates your idea that we don't
add pid/credential references when we are not requesting credentials
in the socket. And probably another one that remove unnecessary
pid/credentials references in send/receive when we do use credentials?
Tim
Eric,
Can you re-spin a patch that incorporates your idea that we don't
add pid/credential references when we are not requesting credentials
in the socket. And probably another one that remove unnecessary
pid/credentials references in send/receive when we do use credentials?
Sure, I did it this morning, please review it if you have some time.
If we find a regression (some popular app doing write() and expecting
credential to be sent to receiver), we could test the SOCK_PASSCRED flag
on receiver socket.