From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=eKFbCl3u; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id 8289C5A068E for ; Thu, 18 Sep 2025 09:17:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1758179839; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=X9KZNHofLg+ybWAspWLP0uErjGstPUm7yQYqXMMPiiI=; b=eKFbCl3uINKJK1x3C17/dzCsWxskPxXQzFGJutZbGRIpcfie+MC3VbOT+7SZj2RDWbIXdQ kF7VVeyiQCw4/iqp+eJabhJ4Sbu6VEN4XHeq29mBbAfdFn54ZAiRkYgEe0SGy7d/oUHrSV iBdNHKmwEnVaTWlH+7c8D8o+ZLy/l3o= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-323-ezrID1dUPjuDTso3kdxhjw-1; Thu, 18 Sep 2025 03:17:17 -0400 X-MC-Unique: ezrID1dUPjuDTso3kdxhjw-1 X-Mimecast-MFC-AGG-ID: ezrID1dUPjuDTso3kdxhjw_1758179837 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-45b9a856d58so4015865e9.0 for ; Thu, 18 Sep 2025 00:17:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1758179836; x=1758784636; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=X9KZNHofLg+ybWAspWLP0uErjGstPUm7yQYqXMMPiiI=; b=LpTwOsHIxDZ7qXTtfPQlnmqqlDNabDdevC2wxxaCjrllB6aj2XFQ4/GnVbRwrVxX95 lqINCSka8OusDARRvGwNC6C9JNlxS7/3/O5gJ6Cu0aASe4f9p2nE84SAeHqyKaN7uEsX fl6TDeJjC+iTpfWE70K6g3LMs4Fx9mzR/cQWdyxS3oVqc5SsNa1K//5wJaeGURATMygS s2j8cTUzPLW/svNSGz+r8Amjhj/PvEh8Ptm8eJKpQ2K7zhKZEsoBJXYuazcqggNv7W5V 6KSG3j1XXMfOWw8KAQWS1wPUOAPhSFgSg9An7K9zGHlzRrmCM5kb0Bdcuddr88K1tHKc yuDw== X-Forwarded-Encrypted: i=1; AJvYcCVQ4Tu9ayKLn6fmXHv4IMvEDvXixnVLfy1nJF1gJgJh3wWrW+dK1kbAzknFNQfGA7D3772od87MVOc=@passt.top X-Gm-Message-State: AOJu0YzCxajYocLFjEG0MjryA33/POkNmfv+DBPskSLY8qSvhXdmHadB HA2H9tCZkgcSKs8BYUXa9wiXoYOvn0t3wJzvq1S1TsNU/iisFeKx9EfJ9prs/4ppCRzzXejsBHO Y7G7luWSqnmwXdNe9wI2AjNoy5/6JBrae+qwkJ99SyPXZUnZvcB/kKA== X-Gm-Gg: ASbGnctdwWkWXqWl2nNwIna3OpShuK1gTTxme4QvnUrZERNUeuXgffa2jZWgnyvOOVz WFif9WeomrTSPNsVB8J7MRFS2tp8LEJJn1p+0Aj2p96fRUTa2jPL6PxqdCSre/Jj4nQahjhvXw6 vo5/LQ32GgNNAe9Vda664c2XqOll8O6cEUntBlTVwuTNoC5ZnNn32uhvkpt2B7sOGn2ZzR8Z2jk VlM2M3Zc7eol2nW9aeDAM7P4Q4wRwDHgVBJ6PVQqJ+2pVOHHRVEcnivRvE7hwYLKFWYR8Iuxyc0 6JMi1zxGiw1Ybh8qKCuS7jyx5N90P5w7ZnYBh82s+3JDPae5sPw= X-Received: by 2002:a05:600c:314b:b0:459:d3ce:2cbd with SMTP id 5b1f17b1804b1-46202bf8474mr42841765e9.13.1758179836371; Thu, 18 Sep 2025 00:17:16 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEWn9ryXPaXipuolbDvMheBYy/fyIUEWxOoQakE/0ztuEC9OFfG3wQH+3+dcbLFerQmAfqqwQ== X-Received: by 2002:a05:600c:314b:b0:459:d3ce:2cbd with SMTP id 5b1f17b1804b1-46202bf8474mr42841545e9.13.1758179835767; Thu, 18 Sep 2025 00:17:15 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4613e754140sm66703605e9.21.2025.09.18.00.17.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 Sep 2025 00:17:15 -0700 (PDT) Date: Thu, 18 Sep 2025 09:17:14 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH] tap: Drop frames if no client connected Message-ID: <20250918091714.77192b00@elisabeth> In-Reply-To: References: <20250911085519.24395-1-yuhuang@redhat.com> <20250911115425.79eaaac5@elisabeth> <20250915081319.00e72e53@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 7c4T7apw2559RAZf3-B3HoGpS0xcdakVwduVESEHhQ0_1758179837 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: KODYEO7SMYLYYENFE2JGQYXBHFAAQXOP X-Message-ID-Hash: KODYEO7SMYLYYENFE2JGQYXBHFAAQXOP X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Yumei Huang , passt-dev@passt.top, lvivier@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, 18 Sep 2025 14:28:37 +1000 David Gibson wrote: > On Mon, Sep 15, 2025 at 08:13:19AM +0200, Stefano Brivio wrote: > > On Fri, 12 Sep 2025 12:01:37 +1000 > > David Gibson wrote: > > > > > On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote: > > > > On Thu, 11 Sep 2025 16:55:19 +0800 > > > > Yumei Huang wrote: > > > > > > > > > If no client is attached, discard outgoing frames and report them as > > > > > sent. This mimics the behavior of a physical host with its network > > > > > cable unplugged. > > > > > > > > > > Suggested-by: David Gibson > > > > > Signed-off-by: Yumei Huang > > > > > > > > Thanks, the fix itself obviously makes sense, but I have a few questions > > > > and comments: > > > > > > > > - first off, what happens if we don't return early in tap_send_frames()? > > > > Commit messages for fixes (assuming this is a fix) should always say > > > > what concrete problem we had, what is going to be fixed, or if we're > > > > not aware of any real issue but things are just fragile / wrong > > > > > > Without this we will get an EBADF in either writev() (pasta) or > > > sendmsg() (passt). That's basically harmless, but a bit ugly. > > > Explicitly catching this case results in behaviour that's probably a > > > bit clearer to debug if we hit it. > > > > > > Putting that context in the commit message would be useful. > > > > > > > - until a while ago, this couldn't happen at all. We were just blocking > > > > the whole execution as long as the tap / guest / container interface > > > > wasn't up and running. > > > > > > > > I wonder when this changed and if it makes sense to go back to the > > > > previous behaviour. I had just a quick look and I wonder if I > > > > accidentally broke this in c9b241346569 ("conf, passt, tap: Open > > > > socket and PID files before switching UID/GID"). > > > > > > > > Before that, main() would call tap_sock_init(), which would call > > > > tap_sock_unix_open(), a blocking function. > > > > > > > > Should we make the whole thing blocking again? If not, is there > > > > anything else that's breaking with that? Timers, other inputs, etc. > > > > > > I don't think we can quite do that. I'm not sure if it's the only > > > reason, but for vhost-user I believe we need the epoll loop up and > > > running before we have the tap connection fully set up, because we > > > need it to process the vhost-user control messages. Laurent, can you > > > verify? > > > > We discussed this in the past, before realising that the execution > > continues for whatever reason, and probably before I broke the > > assumption that guest connection was blocking. > > > > Yes, in the vhost-user case, the epoll loop needs to run before we have > > a working connection to the guest, but: > > > > - we can anyway block until the control socket is set up (we used to do > > that) > > The vhost-user control socket? I'm not entirely sure what you mean by > "block" here. Since we need the epoll loop up, I don't see how we can > block in the conventional sense. Let's rather say "until the data setup is complete". And by "block", I mean we would ignore any other event, obviously we have to listen to the control socket (in the main loop or in a separate dedicated loop). I'm not suggesting we do this though (see below). It's just a possibility. > > > - the vhost-user implementation autonomously throws data away received > > before that point > > Right. It doesn't have anywhere to put it, so it doesn't have much > choice. > > > Now, I don't think we necessarily need to stick to that approach, it > > was the obvious choice when passt was much simpler, and it keeps things > > simple in the sense that we don't need to care about cases like the > > ones this patch is addressing. > > > > On the other hand, if we want to switch to a different model, we need > > to have a look at other possible breakages, I guess. > > > > > There are several different approaches we can take here. I discussed > > > some with Yumei and suggested she take this one. Here's some > > > reasoning (maybe this would also be useful in the commit message, > > > though it's rather bulky) > > > > > > # Don't listen() until the tap connection is ready > > > > > > - It's not clear that the host rejecting the connection is better > > > than the host accepting, then the connection stalling until the > > > guest is ready. > > > - Would require substantial rework because we currently listen() as > > > we parse the command line and don't store the information we'd need > > > to do it later. > > > > Right, that looks like a lot of effort for nothing. > > > > > # Don't accept() until the tap connection is ready > > > > > > - To the peer, will behave basically the same as this patch - the > > > host will complete the TCP handshake, then the connection will stall > > > until the guest is ready. > > > > Same here. > > > > > - More work to implement, because essentially every sock-side handler > > > has to check fd_tap and abort early > > > > There's one substantial issue at TCP level, though, that we're keeping > > with the current approach and with this patch: we'll accept inbound > > connections and silently stall them. > > > > We could mitigate that by making the TCP handler aware of this, and by > > resetting the connection if the guest isn't there. This would at least > > be consistent with the case where the guest isn't listening on the port > > (we accept(), fail to connect to it, eventually call tcp_rst()). > > True. Arguably less consistent with a non-passt-connected peer that's > not there though. Plus with the silently stall approach we have a > chance that the TCP connection will recover if the guest attaches > reasonably soon. > > > If we don't do this, I think we should at least check what happens in > > terms of race conditions between passt starting and the guest appearing > > and accepting the connection. I guess we'll retry for a bit, which is > > desirable, but we should check that the whole retrying thing actually > > works. > > > > That's because the current approach just happened by accident. > > Right. I'm not entirely sure what concrete action you're suggesting > at this point, though. What I suggested in Monday's call and seemed to be all agreed upon, and also mentioned above: *check what happens*. Try that case, with this patch. Does it work to cover situations where users might start passt a bit before the guest connects, and try to connect to services right away? I suggested using ssh which should have a quite long timeout and retry connecting for a while. You mentioned you would assist Yumei in testing this if needed. -- Stefano