diff mbox series

[v2,1/4] net: Split a __sys_bind helper for io_uring

Message ID 20240614163047.31581-1-krisman@suse.de (mailing list archive)
State New
Headers show
Series [v2,1/4] net: Split a __sys_bind helper for io_uring | expand

Commit Message

Gabriel Krisman Bertazi June 14, 2024, 4:30 p.m. UTC 
io_uring holds a reference to the file and maintains a
sockaddr_storage address.  Similarly to what was done to
__sys_connect_file, split an internal helper for __sys_bind in
preparation to supporting an io_uring bind command.

Reviewed-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 include/linux/socket.h |  2 ++
 net/socket.c           | 25 ++++++++++++++++---------
 2 files changed, 18 insertions(+), 9 deletions(-)

Comments

Kuniyuki Iwashima June 14, 2024, 10:42 p.m. UTC | #1 
From: Gabriel Krisman Bertazi <krisman@suse.de>
Date: Fri, 14 Jun 2024 12:30:44 -0400
> io_uring holds a reference to the file and maintains a
> sockaddr_storage address.  Similarly to what was done to
> __sys_connect_file, split an internal helper for __sys_bind in
> preparation to supporting an io_uring bind command.
> 
> Reviewed-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Jakub Kicinski June 19, 2024, 12:49 a.m. UTC | #2 
On Fri, 14 Jun 2024 12:30:44 -0400 Gabriel Krisman Bertazi wrote:
> io_uring holds a reference to the file and maintains a
> sockaddr_storage address.  Similarly to what was done to
> __sys_connect_file, split an internal helper for __sys_bind in
> preparation to supporting an io_uring bind command.
> 
> Reviewed-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>

Acked-by: Jakub Kicinski <kuba@kernel.org>
Jens Axboe June 19, 2024, 1:40 p.m. UTC | #3 
On 6/18/24 6:49 PM, Jakub Kicinski wrote:
> On Fri, 14 Jun 2024 12:30:44 -0400 Gabriel Krisman Bertazi wrote:
>> io_uring holds a reference to the file and maintains a
>> sockaddr_storage address.  Similarly to what was done to
>> __sys_connect_file, split an internal helper for __sys_bind in
>> preparation to supporting an io_uring bind command.
>>
>> Reviewed-by: Jens Axboe <axboe@kernel.dk>
>> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
> 
> Acked-by: Jakub Kicinski <kuba@kernel.org>

Are you fine with me queueing up 1-2 via the io_uring branch?
I'm guessing the risk of conflict should be very low, so doesn't
warrant a shared branch.
Jens Axboe June 19, 2024, 2:58 p.m. UTC | #4 
On Fri, 14 Jun 2024 12:30:44 -0400, Gabriel Krisman Bertazi wrote:
> io_uring holds a reference to the file and maintains a
> sockaddr_storage address.  Similarly to what was done to
> __sys_connect_file, split an internal helper for __sys_bind in
> preparation to supporting an io_uring bind command.
> 
> 

Applied, thanks!

[1/4] net: Split a __sys_bind helper for io_uring
      commit: dc2e77979412d289df9049d8c693761db8602867
[2/4] net: Split a __sys_listen helper for io_uring
      commit: bb6aaf736680f0f3c2e6281735c47c64e2042819
[3/4] io_uring: Introduce IORING_OP_BIND
      commit: 7481fd93fa0a851740e26026485f56a1305454ce
[4/4] io_uring: Introduce IORING_OP_LISTEN
      commit: ff140cc8628abfb1755691d16cfa8788d8820ef7

Best regards,
Jakub Kicinski June 19, 2024, 3:04 p.m. UTC | #5 
On Wed, 19 Jun 2024 07:40:40 -0600 Jens Axboe wrote:
> On 6/18/24 6:49 PM, Jakub Kicinski wrote:
> > On Fri, 14 Jun 2024 12:30:44 -0400 Gabriel Krisman Bertazi wrote:  
> >> io_uring holds a reference to the file and maintains a
> >> sockaddr_storage address.  Similarly to what was done to
> >> __sys_connect_file, split an internal helper for __sys_bind in
> >> preparation to supporting an io_uring bind command.
> >>
> >> Reviewed-by: Jens Axboe <axboe@kernel.dk>
> >> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>  
> > 
> > Acked-by: Jakub Kicinski <kuba@kernel.org>  
> 
> Are you fine with me queueing up 1-2 via the io_uring branch?
> I'm guessing the risk of conflict should be very low, so doesn't
> warrant a shared branch.

Yup, exactly, these can go via io_uring without branch juggling.
Jens Axboe June 19, 2024, 3:06 p.m. UTC | #6 
On 6/19/24 9:04 AM, Jakub Kicinski wrote:
> On Wed, 19 Jun 2024 07:40:40 -0600 Jens Axboe wrote:
>> On 6/18/24 6:49 PM, Jakub Kicinski wrote:
>>> On Fri, 14 Jun 2024 12:30:44 -0400 Gabriel Krisman Bertazi wrote:  
>>>> io_uring holds a reference to the file and maintains a
>>>> sockaddr_storage address.  Similarly to what was done to
>>>> __sys_connect_file, split an internal helper for __sys_bind in
>>>> preparation to supporting an io_uring bind command.
>>>>
>>>> Reviewed-by: Jens Axboe <axboe@kernel.dk>
>>>> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>  
>>>
>>> Acked-by: Jakub Kicinski <kuba@kernel.org>  
>>
>> Are you fine with me queueing up 1-2 via the io_uring branch?
>> I'm guessing the risk of conflict should be very low, so doesn't
>> warrant a shared branch.
> 
> Yup, exactly, these can go via io_uring without branch juggling.

Great thanks!
Matus Jokay July 15, 2024, 12:49 p.m. UTC | #7 
On 19. 6. 2024 17:06, Jens Axboe wrote:
> On 6/19/24 9:04 AM, Jakub Kicinski wrote:
>> On Wed, 19 Jun 2024 07:40:40 -0600 Jens Axboe wrote:
>>> On 6/18/24 6:49 PM, Jakub Kicinski wrote:
>>>> On Fri, 14 Jun 2024 12:30:44 -0400 Gabriel Krisman Bertazi wrote:  
>>>>> io_uring holds a reference to the file and maintains a
>>>>> sockaddr_storage address.  Similarly to what was done to
>>>>> __sys_connect_file, split an internal helper for __sys_bind in
>>>>> preparation to supporting an io_uring bind command.
>>>>>
>>>>> Reviewed-by: Jens Axboe <axboe@kernel.dk>
>>>>> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>  
>>>>
>>>> Acked-by: Jakub Kicinski <kuba@kernel.org>  
>>>
>>> Are you fine with me queueing up 1-2 via the io_uring branch?
>>> I'm guessing the risk of conflict should be very low, so doesn't
>>> warrant a shared branch.
>>
>> Yup, exactly, these can go via io_uring without branch juggling.
> 
> Great thanks!
> 
Please fix io_bind and io_listen to not pass NULL ptr to related helpers
__sys_bind_socket and __sys_listen_socket. The first helper's argument
shouldn't be NULL, as related security hooks expect a valid socket object.

See the syzkaller's bug report:
https://lore.kernel.org/linux-security-module/0000000000007b7ce6061d1caec0@google.com/

Thanks,
mY
Tetsuo Handa July 15, 2024, 1:59 p.m. UTC | #8 
On 2024/07/15 21:49, Matus Jokay wrote:
> Please fix io_bind and io_listen to not pass NULL ptr to related helpers
> __sys_bind_socket and __sys_listen_socket. The first helper's argument
> shouldn't be NULL, as related security hooks expect a valid socket object.
> 
> See the syzkaller's bug report:
> https://lore.kernel.org/linux-security-module/0000000000007b7ce6061d1caec0@google.com/

That was already fixed.

https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/commit/?h=for-next&id=ad00e629145b2b9f0d78aa46e204a9df7d628978
Matus Jokay July 22, 2024, 12:37 p.m. UTC | #9 
On 15. 7. 2024 15:59, Tetsuo Handa wrote:
> On 2024/07/15 21:49, Matus Jokay wrote:
>> Please fix io_bind and io_listen to not pass NULL ptr to related helpers
>> __sys_bind_socket and __sys_listen_socket. The first helper's argument
>> shouldn't be NULL, as related security hooks expect a valid socket object.
>>
>> See the syzkaller's bug report:
>> https://lore.kernel.org/linux-security-module/0000000000007b7ce6061d1caec0@google.com/
> 
> That was already fixed.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/commit/?h=for-next&id=ad00e629145b2b9f0d78aa46e204a9df7d628978
> 
> 
Tetsuo, you are very fast ;)
Thank you!
mY
diff mbox series

Patch

diff --git a/include/linux/socket.h b/include/linux/socket.h
index 89d16b90370b..b3000f49e9f5 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -442,6 +442,8 @@  extern int __sys_accept4(int fd, struct sockaddr __user *upeer_sockaddr,
 extern int __sys_socket(int family, int type, int protocol);
 extern struct file *__sys_socket_file(int family, int type, int protocol);
 extern int __sys_bind(int fd, struct sockaddr __user *umyaddr, int addrlen);
+extern int __sys_bind_socket(struct socket *sock, struct sockaddr_storage *address,
+			     int addrlen);
 extern int __sys_connect_file(struct file *file, struct sockaddr_storage *addr,
 			      int addrlen, int file_flags);
 extern int __sys_connect(int fd, struct sockaddr __user *uservaddr,
diff --git a/net/socket.c b/net/socket.c
index e416920e9399..fd0714e10ced 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1822,6 +1822,20 @@  SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,
 	return __sys_socketpair(family, type, protocol, usockvec);
 }
 
+int __sys_bind_socket(struct socket *sock, struct sockaddr_storage *address,
+		      int addrlen)
+{
+	int err;
+
+	err = security_socket_bind(sock, (struct sockaddr *)address,
+				   addrlen);
+	if (!err)
+		err = READ_ONCE(sock->ops)->bind(sock,
+						 (struct sockaddr *)address,
+						 addrlen);
+	return err;
+}
+
 /*
  *	Bind a name to a socket. Nothing much to do here since it's
  *	the protocol's responsibility to handle the local address.
@@ -1839,15 +1853,8 @@  int __sys_bind(int fd, struct sockaddr __user *umyaddr, int addrlen)
 	sock = sockfd_lookup_light(fd, &err, &fput_needed);
 	if (sock) {
 		err = move_addr_to_kernel(umyaddr, addrlen, &address);
-		if (!err) {
-			err = security_socket_bind(sock,
-						   (struct sockaddr *)&address,
-						   addrlen);
-			if (!err)
-				err = READ_ONCE(sock->ops)->bind(sock,
-						      (struct sockaddr *)
-						      &address, addrlen);
-		}
+		if (!err)
+			err = __sys_bind_socket(sock, &address, addrlen);
 		fput_light(sock->file, fput_needed);
 	}
 	return err;