axboe / liburing

Library providing helpers for the Linux kernel io_uring support
MIT License
2.85k stars 402 forks source link

behavior change when writing into a socket with its counter-part closed - no EPIPE signal is sent #1015

Closed romange closed 10 months ago

romange commented 10 months ago

the minimal reproducible example is below. I verified this bug/behavior change exist on kernels 6.2 and 6.5 (ubuntu 23.10).

  struct io_uring ring;
  ASSERT_EQ(0, io_uring_queue_init(8, &ring, 0));
  constexpr auto kMask = SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC;
  int fd = socket(AF_INET, kMask, 0);  // listening socket
  ASSERT_GE(fd, 0);

  const int val = 1;
  setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));

  int res;

  struct sockaddr_in addr;
  memset(&addr, 0, sizeof(addr));
  addr.sin_family = AF_INET;
  addr.sin_port = 1234;
  addr.sin_addr.s_addr = INADDR_ANY;

  res = bind(fd, (struct sockaddr*)&addr, sizeof(addr));
  ASSERT_EQ(0, res);
  res = listen(fd, 128);
  ASSERT_EQ(0, res);

  // client socket
  int cfd = socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC, IPPROTO_TCP);
  ASSERT_GT(cfd, 0);

  res = connect(cfd, (struct sockaddr*)&addr, sizeof(addr));
  ASSERT_EQ(-1, res);
  ASSERT_EQ(EINPROGRESS, errno);
  res = accept4(fd, NULL, NULL, SOCK_NONBLOCK | SOCK_CLOEXEC);

  ASSERT_GT(res, 0);
  int sfd = res;   // created a server socket

  res = connect(cfd, (struct sockaddr*)&addr, sizeof(addr)); 
  ASSERT_EQ(0, res);   // verify that client socket is connected.

 #define REPRODUCE_BUG 1   // with this section enabled the line (*) fails below.
 #if  REPRODUCE_BUG   
  res = io_uring_register_files(&ring, &sfd, 1);
  ASSERT_EQ(0, res);
  int update_fd = -1;
  res = io_uring_register_files_update(&ring, 0, &update_fd, 1);
  ASSERT_EQ(1, res);
#endif 

  close(sfd);

  char buf[128];
  send(cfd, buf, sizeof(buf), MSG_NOSIGNAL);
  res = send(cfd, buf, sizeof(buf), MSG_NOSIGNAL);

  // (*) this holds with regular sockets because we closed sfd
   //  but if we register/unregister sfd using ioring  and then close sfd, we do not receive the EPIPE error for some reason.
  ASSERT_EQ(-1, res);
  ASSERT_EQ(EPIPE, errno);  

  close(fd);
  close(cfd);
  io_uring_queue_exit(&ring);
axboe commented 10 months ago

Before I look into this, the report seems to indicate that this is a change/regression, but doesn't mention what the working version was?

axboe commented 10 months ago

I guess looking at the reproducer, this is a registered vs normal file descriptor thing?

romange commented 10 months ago

yes, the normal behavior can be achieved by commenting out the registering/unregistering lines. so both versions work with regular file descriptors, the behavior change is caused by registering a file descriptor with iouring.

axboe commented 10 months ago

I'll take a look, but my guess would be that it's because there's a reference left on it, which will get dropped eventually. At least that's the only reason why I could see this making a difference.

axboe commented 10 months ago

Doesn't reproduce for me on 6.4-stable, 6.5-stable, or current -git...

axboe commented 10 months ago

Tried 6.2-stable and it does fail there. But 6.2-stable has been dead since May this year, so no more releases of that will get made. For anything current, I don't see the issue. I'm surprised you then see it on 6.5, which is some ubuntu variant I suppose?

romange commented 10 months ago

Thanks, I will double check it again on 6.5 (ubuntu 23.10)

axboe commented 10 months ago

My suspicion is that this got fixed with the resource rework that Pavel did for 6.4, so anything earlier than 6.4 would likely not pass your test case. Which does mean that 6.5 really should work, however...

axboe commented 10 months ago

Yep I think that's what it is. It'll likely work if you have a 2 second delay in there after the unregister. Not that this is super useful information in terms of making it work generically, but you could confirm that with adding a sleep(2); after the io_uring_register_files_update() call.

romange commented 10 months ago

Yes, it's my mistake - it's working well on 6.5.