netdev - Re: [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 24 Sep 2021 02:39:49 +0000
From:   yajun.deng@...ux.dev
To:     "Jakub Kicinski" <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: socket: integrate sockfd_lookup() and
 sockfd_lookup_light()

September 23, 2021 11:24 PM, "Jakub Kicinski" <kuba@...nel.org> wrote:

> On Wed, 22 Sep 2021 14:31:06 +0800 Yajun Deng wrote:
> 
>> As commit 6cb153cab92a("[NET]: use fget_light() in net/socket.c") said,
>> sockfd_lookup_light() is lower load than sockfd_lookup(). So we can
>> remove sockfd_lookup() but keep the name. As the same time, move flags
>> to sockfd_put().
> 
> You just assume that each caller of sockfd_lookup() already meets the
> criteria under which sockfd_lookup_light() can be used? Am I reading
> this right?
> 
Yes, this patch means each caller of sockfd_lookup() can used sockfd_lookup_light() instead.
> Please extend the commit message clearly walking us thru why this is
> safe now (and perhaps why it wasn't in the past).
>
The sockfd_lookup() and  sockfd_lookup_light() are both safe. The fact that they have been around for so long is the best proof. sockfd_lookup_light() is just lower load than sockfd_lookup(). so we can used the lower load helper function.

>> static ssize_t sockfs_listxattr(struct dentry *dentry, char *buffer,
>> size_t size)
>> @@ -1680,9 +1659,9 @@ int __sys_bind(int fd, struct sockaddr __user *umyaddr, int addrlen)
>> {
>> struct socket *sock;
>> struct sockaddr_storage address;
>> - int err, fput_needed;
>> + int err;
>> 
>> - sock = sockfd_lookup_light(fd, &err, &fput_needed);
>> + sock = sockfd_lookup(fd, &err);
>> if (sock) {
>> err = move_addr_to_kernel(umyaddr, addrlen, &address);
>> if (!err) {
>> @@ -1694,7 +1673,7 @@ int __sys_bind(int fd, struct sockaddr __user *umyaddr, int addrlen)
>> (struct sockaddr *)
>> &address, addrlen);
>> }
>> - fput_light(sock->file, fput_needed);
>> + sockfd_put(sock);
> 
> And we just replace fput_light() with fput() even tho the reference was
> taken with fdget()? fdget() == __fget_light().
> 
> Maybe you missed fget() vs fdget()?

In fact, the sockfd_put() already changed in this patch. Here is the modified:
#define                     sockfd_put(sock)             \
do {                                              \
       struct fd *fd = (struct fd *)&sock->file; \
                                                 \
       if (fd->flags & FDPUT_FPUT)               \
               fput(sock->file);                 \
}

> 
> All these changes do not immediately strike me as correct.
> 
This is the information of this patch:
 include/linux/net.h |   8 +++-
 net/socket.c        | 101 +++++++++++++++++---------------------------
 2 files changed, 46 insertions(+), 63 deletions(-)

>> }
>> return err;
>> }