* [Eulerkernel] [PATCH] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL @ 2013-03-25 10:28 dingtianhong 2013-03-25 14:04 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: dingtianhong @ 2013-03-25 10:28 UTC (permalink / raw) To: David S. Miller, Eric Dumazet, netdev, Li Zefan, Xinwei Hu 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(); } -- ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Eulerkernel] [PATCH] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL 2013-03-25 10:28 [Eulerkernel] [PATCH] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL dingtianhong @ 2013-03-25 14:04 ` Eric Dumazet 2013-03-25 17:12 ` David Miller 2013-03-26 3:08 ` dingtianhong 0 siblings, 2 replies; 8+ messages in thread From: Eric Dumazet @ 2013-03-25 14:04 UTC (permalink / raw) To: dingtianhong; +Cc: David S. Miller, Eric Dumazet, netdev, Li Zefan, Xinwei Hu 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Eulerkernel] [PATCH] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL 2013-03-25 14:04 ` Eric Dumazet @ 2013-03-25 17:12 ` David Miller 2013-03-26 3:08 ` dingtianhong 1 sibling, 0 replies; 8+ messages in thread From: David Miller @ 2013-03-25 17:12 UTC (permalink / raw) To: eric.dumazet; +Cc: dingtianhong, edumazet, netdev, lizefan, huxinwei 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Eulerkernel] [PATCH] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL 2013-03-25 14:04 ` Eric Dumazet 2013-03-25 17:12 ` David Miller @ 2013-03-26 3:08 ` dingtianhong 2013-03-26 4:32 ` Eric Dumazet 1 sibling, 1 reply; 8+ messages in thread From: dingtianhong @ 2013-03-26 3:08 UTC (permalink / raw) To: Eric Dumazet; +Cc: David S. Miller, Eric Dumazet, netdev, Li Zefan, Xinwei Hu 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> > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Eulerkernel] [PATCH] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL 2013-03-26 3:08 ` dingtianhong @ 2013-03-26 4:32 ` Eric Dumazet 2013-03-26 11:35 ` dingtianhong 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2013-03-26 4:32 UTC (permalink / raw) To: dingtianhong; +Cc: David S. Miller, Eric Dumazet, netdev, Li Zefan, Xinwei Hu 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Eulerkernel] [PATCH] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL 2013-03-26 4:32 ` Eric Dumazet @ 2013-03-26 11:35 ` dingtianhong 2013-03-26 13:46 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: dingtianhong @ 2013-03-26 11:35 UTC (permalink / raw) To: Eric Dumazet; +Cc: David S. Miller, Eric Dumazet, netdev, Li Zefan, Xinwei Hu 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; } > > > > . > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Eulerkernel] [PATCH] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL 2013-03-26 11:35 ` dingtianhong @ 2013-03-26 13:46 ` Eric Dumazet 2013-03-27 7:35 ` dingtianhong 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2013-03-26 13:46 UTC (permalink / raw) To: dingtianhong; +Cc: David S. Miller, Eric Dumazet, netdev, Li Zefan, Xinwei Hu 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 ? ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL 2013-03-26 13:46 ` Eric Dumazet @ 2013-03-27 7:35 ` dingtianhong 0 siblings, 0 replies; 8+ messages in thread From: dingtianhong @ 2013-03-27 7:35 UTC (permalink / raw) To: Eric Dumazet; +Cc: David S. Miller, Eric Dumazet, netdev, Li Zefan, Xinwei Hu 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. > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-03-27 7:36 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-25 10:28 [Eulerkernel] [PATCH] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL dingtianhong 2013-03-25 14:04 ` Eric Dumazet 2013-03-25 17:12 ` David Miller 2013-03-26 3:08 ` dingtianhong 2013-03-26 4:32 ` Eric Dumazet 2013-03-26 11:35 ` dingtianhong 2013-03-26 13:46 ` Eric Dumazet 2013-03-27 7:35 ` dingtianhong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).