| Message ID | 515026DA.7050306@huawei.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2013-03-25 at 18:28 +0800, dingtianhong wrote: > SCM_SCREDENTIALS should apply to write() syscalls only either source or destination > socket asserted SOCK_PASSCRED. The original implememtation in maybe_add_creds is wrong, > and breaks several LSB testcases ( i.e. /tset/LSB.os/netowkr/recvfrom/T.recvfrom). > > Origionally-authored-by: Karel Srot <ksrot@redhat.com> > Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> > --- > net/unix/af_unix.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index 51be64f..99189fd 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock, > if (UNIXCB(skb).cred) > return; > if (test_bit(SOCK_PASSCRED, &sock->flags) || > - !other->sk_socket || > - test_bit(SOCK_PASSCRED, &other->sk_socket->flags)) { > + (other->sk_socket && > + test_bit(SOCK_PASSCRED, &other->sk_socket->flags))) { > UNIXCB(skb).pid = get_pid(task_tgid(current)); > UNIXCB(skb).cred = get_current_cred(); > } I am not sure why adding credentials if other->sk_socket is NULL could break an application ? This was the case before commit introducing this code. Anyway patch looks fine Acked-by: Eric Dumazet <edumazet@google.com> -- 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 <eric.dumazet@gmail.com> Date: Mon, 25 Mar 2013 07:04:29 -0700 > On Mon, 2013-03-25 at 18:28 +0800, dingtianhong wrote: >> SCM_SCREDENTIALS should apply to write() syscalls only either source or destination >> socket asserted SOCK_PASSCRED. The original implememtation in maybe_add_creds is wrong, >> and breaks several LSB testcases ( i.e. /tset/LSB.os/netowkr/recvfrom/T.recvfrom). >> >> Origionally-authored-by: Karel Srot <ksrot@redhat.com> >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >> --- >> net/unix/af_unix.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c >> index 51be64f..99189fd 100644 >> --- a/net/unix/af_unix.c >> +++ b/net/unix/af_unix.c >> @@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock, >> if (UNIXCB(skb).cred) >> return; >> if (test_bit(SOCK_PASSCRED, &sock->flags) || >> - !other->sk_socket || >> - test_bit(SOCK_PASSCRED, &other->sk_socket->flags)) { >> + (other->sk_socket && >> + test_bit(SOCK_PASSCRED, &other->sk_socket->flags))) { >> UNIXCB(skb).pid = get_pid(task_tgid(current)); >> UNIXCB(skb).cred = get_current_cred(); >> } > > I am not sure why adding credentials if other->sk_socket is NULL could > break an application ? > > This was the case before commit introducing this code. > > Anyway patch looks fine > > Acked-by: Eric Dumazet <edumazet@google.com> This patch is massively whitespace damaged, and therefore completely unusable. Please fix this, and only resubmit this patch when you can successfully email the patch to yourself and apply it successfully. -- 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
On 2013/3/25 22:04, Eric Dumazet wrote: > On Mon, 2013-03-25 at 18:28 +0800, dingtianhong wrote: >> SCM_SCREDENTIALS should apply to write() syscalls only either source or destination >> socket asserted SOCK_PASSCRED. The original implememtation in maybe_add_creds is wrong, >> and breaks several LSB testcases ( i.e. /tset/LSB.os/netowkr/recvfrom/T.recvfrom). >> >> Origionally-authored-by: Karel Srot <ksrot@redhat.com> >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >> --- >> net/unix/af_unix.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c >> index 51be64f..99189fd 100644 >> --- a/net/unix/af_unix.c >> +++ b/net/unix/af_unix.c >> @@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock, >> if (UNIXCB(skb).cred) >> return; >> if (test_bit(SOCK_PASSCRED, &sock->flags) || >> - !other->sk_socket || >> - test_bit(SOCK_PASSCRED, &other->sk_socket->flags)) { >> + (other->sk_socket && >> + test_bit(SOCK_PASSCRED, &other->sk_socket->flags))) { >> UNIXCB(skb).pid = get_pid(task_tgid(current)); >> UNIXCB(skb).cred = get_current_cred(); >> } > > I am not sure why adding credentials if other->sk_socket is NULL could > break an application ? The bugzilla has report the bug:https://lsbbugs.linuxfoundation.org/show_bug.cgi?id=3523 > > This was the case before commit introducing this code. The commit 16e5726269(af_unix: dont send SCM_CREDENTIALS by default) may introducing the problem. > > Anyway patch looks fine > > Acked-by: Eric Dumazet <edumazet@google.com> > > > > > > -- 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
On Tue, 2013-03-26 at 11:08 +0800, dingtianhong wrote: > On 2013/3/25 22:04, Eric Dumazet wrote: > > On Mon, 2013-03-25 at 18:28 +0800, dingtianhong wrote: > >> SCM_SCREDENTIALS should apply to write() syscalls only either source or destination > >> socket asserted SOCK_PASSCRED. The original implememtation in maybe_add_creds is wrong, > >> and breaks several LSB testcases ( i.e. /tset/LSB.os/netowkr/recvfrom/T.recvfrom). > >> > >> Origionally-authored-by: Karel Srot <ksrot@redhat.com> > >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> > >> --- > >> net/unix/af_unix.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > >> index 51be64f..99189fd 100644 > >> --- a/net/unix/af_unix.c > >> +++ b/net/unix/af_unix.c > >> @@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock, > >> if (UNIXCB(skb).cred) > >> return; > >> if (test_bit(SOCK_PASSCRED, &sock->flags) || > >> - !other->sk_socket || > >> - test_bit(SOCK_PASSCRED, &other->sk_socket->flags)) { > >> + (other->sk_socket && > >> + test_bit(SOCK_PASSCRED, &other->sk_socket->flags))) { > >> UNIXCB(skb).pid = get_pid(task_tgid(current)); > >> UNIXCB(skb).cred = get_current_cred(); > >> } > > > > I am not sure why adding credentials if other->sk_socket is NULL could > > break an application ? > The bugzilla has report the bug:https://lsbbugs.linuxfoundation.org/show_bug.cgi?id=3523 > OK > > > > This was the case before commit introducing this code. > > The commit 16e5726269(af_unix: dont send SCM_CREDENTIALS by default) may introducing the problem. > So the problem is that two messages have different credentials, because other->sk_socket changed between first and second message. and unix_stream_recvmsg() has the following check : if (check_creds) { /* Never glue messages from different writers */ if ((UNIXCB(skb).pid != siocb->scm->pid) || (UNIXCB(skb).cred != siocb->scm->cred)) break; } else { /* Copy credentials */ scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred); check_creds = 1; } In the case the receiver doesnt care at all (using recvfrom(), not recvmsg()), we probably should not even call scm_set_creds() and avoid extra refcounting. -- 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
On 2013/3/26 12:32, Eric Dumazet wrote: > On Tue, 2013-03-26 at 11:08 +0800, dingtianhong wrote: >> On 2013/3/25 22:04, Eric Dumazet wrote: >>> On Mon, 2013-03-25 at 18:28 +0800, dingtianhong wrote: >>>> SCM_SCREDENTIALS should apply to write() syscalls only either source or destination >>>> socket asserted SOCK_PASSCRED. The original implememtation in maybe_add_creds is wrong, >>>> and breaks several LSB testcases ( i.e. /tset/LSB.os/netowkr/recvfrom/T.recvfrom). >>>> >>>> Origionally-authored-by: Karel Srot <ksrot@redhat.com> >>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >>>> --- >>>> net/unix/af_unix.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c >>>> index 51be64f..99189fd 100644 >>>> --- a/net/unix/af_unix.c >>>> +++ b/net/unix/af_unix.c >>>> @@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock, >>>> if (UNIXCB(skb).cred) >>>> return; >>>> if (test_bit(SOCK_PASSCRED, &sock->flags) || >>>> - !other->sk_socket || >>>> - test_bit(SOCK_PASSCRED, &other->sk_socket->flags)) { >>>> + (other->sk_socket && >>>> + test_bit(SOCK_PASSCRED, &other->sk_socket->flags))) { >>>> UNIXCB(skb).pid = get_pid(task_tgid(current)); >>>> UNIXCB(skb).cred = get_current_cred(); >>>> } >>> >>> I am not sure why adding credentials if other->sk_socket is NULL could >>> break an application ? >> The bugzilla has report the bug:https://lsbbugs.linuxfoundation.org/show_bug.cgi?id=3523 >> > > OK > >>> >>> This was the case before commit introducing this code. >> >> The commit 16e5726269(af_unix: dont send SCM_CREDENTIALS by default) may introducing the problem. >> > > So the problem is that two messages have different credentials, > because other->sk_socket changed between first and second message. > > and unix_stream_recvmsg() has the following check : > > if (check_creds) { > /* Never glue messages from different writers */ > if ((UNIXCB(skb).pid != siocb->scm->pid) || > (UNIXCB(skb).cred != siocb->scm->cred)) > break; > } else { > /* Copy credentials */ > scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred); > check_creds = 1; > } > > In the case the receiver doesnt care at all (using recvfrom(), not recvmsg()), > we probably should not even call scm_set_creds() and avoid extra refcounting. > I think if not call scm_set_creds(), the credential would useles in recvmsg(). we could remove code: if (check_creds) { /* Never glue messages from different writers */ if ((UNIXCB(skb).pid != siocb->scm->pid) || (UNIXCB(skb).cred != siocb->scm->cred)) break; } else { /* Copy credentials */ scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred); check_creds = 1; } > > > > . > -- 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
On Tue, 2013-03-26 at 19:35 +0800, dingtianhong wrote: > I think if not call scm_set_creds(), the credential would useles in recvmsg(). > we could remove code: > if (check_creds) { > /* Never glue messages from different writers */ > if ((UNIXCB(skb).pid != siocb->scm->pid) || > (UNIXCB(skb).cred != siocb->scm->cred)) > break; > } else { > /* Copy credentials */ > scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred); > check_creds = 1; > } Are you paraphrasing me or saying something different ? -- 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
On 2013/3/26 21:46, Eric Dumazet wrote: > On Tue, 2013-03-26 at 19:35 +0800, dingtianhong wrote: > >> I think if not call scm_set_creds(), the credential would useles in recvmsg(). >> we could remove code: >> if (check_creds) { >> /* Never glue messages from different writers */ >> if ((UNIXCB(skb).pid != siocb->scm->pid) || >> (UNIXCB(skb).cred != siocb->scm->cred)) >> break; >> } else { >> /* Copy credentials */ >> scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred); >> check_creds = 1; >> } > > Are you paraphrasing me or saying something different ? > if not call scm_set_creds(), how get credentials for receiver and distinguish different writes, so I think scm_set_creds() is necessary here. > > > -- 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
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 51be64f..99189fd 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock, if (UNIXCB(skb).cred) return; if (test_bit(SOCK_PASSCRED, &sock->flags) || - !other->sk_socket || - test_bit(SOCK_PASSCRED, &other->sk_socket->flags)) { + (other->sk_socket && + test_bit(SOCK_PASSCRED, &other->sk_socket->flags))) { UNIXCB(skb).pid = get_pid(task_tgid(current)); UNIXCB(skb).cred = get_current_cred(); }
SCM_SCREDENTIALS should apply to write() syscalls only either source or destination socket asserted SOCK_PASSCRED. The original implememtation in maybe_add_creds is wrong, and breaks several LSB testcases ( i.e. /tset/LSB.os/netowkr/recvfrom/T.recvfrom). Origionally-authored-by: Karel Srot <ksrot@redhat.com> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> --- net/unix/af_unix.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)