From: Al Viro <viro@ZenIV.linux.org.uk>
To: netdev@vger.kernel.org
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Subject: [RFC][PATCHES] sock_alloc_file() cleanups and fixes
Date: Fri, 1 Dec 2017 00:20:27 +0000 [thread overview]
Message-ID: <20171201002027.GI21978@ZenIV.linux.org.uk> (raw)
Almost all sock_alloc_file() callers want sock_release()
in case of failure. Currently it consumes socket on success
(it will be destroyed on final fput() of resulting file) and
leaves it alone on failure. Making it consume socket in all
cases makes for simpler life in callers.
There are 3 exceptions:
* sock_map_fd() calls sock_alloc_file(), but doesn't do sock_release()
in case of failure. Its caller (sys_socket()) does, though, and it
does get considerably simpler with sock_alloc_file() doing the cleanup
in case of failure.
* sys_socketpair(). Handling of sock_alloc_file() failures is complicated
by attempts to share bits and pieces of failure exits between various
points of failure in there. Reordering things a bit (reserving descriptors
and copying them to userland before doing anything else) makes for simpler
handling of failure exits and after such massage we get the situation
when failure of sock_alloc_file() is immediately followed by sock_release().
* kcm_clone(). Badly broken in several respects - sk_alloc() failure ends
up with double-free of struct socket (we do fput(), then sock_release())
and copy_to_user() failure uses sys_close() to undo fd_install(), which
is something we should never do. Descriptor table might be shared and
fd_install() should only be done past the last possible failure point.
Fixing all of that is simple - we just need to move allocation of
descriptor and fd_install() into the caller (before and after the call of
kcm_clone(), resp.) and untangle the failure exits. Makes for much simpler
calling conventions for kcm_clone(), while we are at it, and as a side
effect we get "sock_release() in case of sock_alloc_file() failure" for
that one as well.
The patch series (in followups to this mail and in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git#work.net) does
the following:
1) massage sys_socketpair() (should be a pure cleanup)
2) fix and clean up kcm_clone() (-stable fodder)
3) switch sock_alloc_file() to new calling conventions.
It got some local testing, but it certainly needs more review.
Diffstat for the entire thing is
drivers/staging/lustre/lnet/lnet/lib-socket.c | 8 ++---
net/9p/trans_fd.c | 1 -
net/kcm/kcmsock.c | 68 ++++++++++++++---------------------------
net/sctp/socket.c | 1 -
net/socket.c | 110 +++++++++++++++++++++++++++----------------------------------------
5 files changed, 69 insertions(+), 119 deletions(-)
Please, review and comment.
next reply other threads:[~2017-12-01 0:20 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-01 0:20 Al Viro [this message]
2017-12-01 0:22 ` [PATCH 1/3] socketpair(): allocate descriptors first Al Viro
2017-12-01 9:28 ` Eric Dumazet
2017-12-01 0:22 ` [PATCH 2/3] fix kcm_clone() Al Viro
2017-12-01 9:31 ` Eric Dumazet
2017-12-01 16:56 ` Tom Herbert
2017-12-01 0:23 ` [PATCH 3/3] make sock_alloc_file() do sock_release() on failures Al Viro
2017-12-01 9:33 ` Eric Dumazet
2017-12-04 15:35 ` [RFC][PATCHES] sock_alloc_file() cleanups and fixes David Miller
2017-12-04 16:41 ` Al Viro
2017-12-05 19:44 ` David Miller
2017-12-05 23:29 ` Al Viro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171201002027.GI21978@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).