From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.nppct.ru (mail.nppct.ru [195.133.245.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A014923A993 for ; Thu, 17 Apr 2025 11:19:52 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=195.133.245.4 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744888798; cv=none; b=ZDSCsbHnhm5OaJwBARbop0yVfuOn4JyyekEqHurtgfbu/LsJKjbF+IRHHQ8zrWv3uci3Dt/gXPvcfWUwNRHZfZr99MzWaHh196AjB+HHB5ik6j0fHsSljZvml6pAkzw4ES4UDUJqcpYsN0vZMJT32Ab1D8wxCa7L6a13YbR/5zI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744888798; c=relaxed/simple; bh=EaA/y+zhy5Or9j5JTA2kf0Ms6ZLviEs08ZXslBXBvVU=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=oXw7tSf6ZUzYWowW5kBHlI7eXIY9qYCOOVeBwnlA7m/hfa8RYm0FZQGbvSLA3+CeANHf9n3Gcq01tXy5tzZsinAr1GGNg9j/7Rc3k2Eqkxe0ReiF3LGb5evI53LJ8suwGCFPYlSLMRanyKtQ+lHZQxLN1XXDmDDuLsRPvxn8OQU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=nppct.ru; spf=pass smtp.mailfrom=nppct.ru; dkim=pass (1024-bit key) header.d=nppct.ru header.i=@nppct.ru header.b=PTv9lybV; arc=none smtp.client-ip=195.133.245.4 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=nppct.ru Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=nppct.ru Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=nppct.ru header.i=@nppct.ru header.b="PTv9lybV" Received: from mail.nppct.ru (localhost [127.0.0.1]) by mail.nppct.ru (Postfix) with ESMTP id 972DE1C0E84 for ; Thu, 17 Apr 2025 14:19:50 +0300 (MSK) Authentication-Results: mail.nppct.ru (amavisd-new); dkim=pass (1024-bit key) reason="pass (just generated, assumed good)" header.d=nppct.ru DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=nppct.ru; h= content-transfer-encoding:content-type:content-type:in-reply-to :from:from:content-language:references:to:subject:subject :user-agent:mime-version:date:date:message-id; s=dkim; t= 1744888790; x=1745752791; bh=EaA/y+zhy5Or9j5JTA2kf0Ms6ZLviEs08ZX slBXBvVU=; b=PTv9lybVRA2uUGNVgliJQYa0VIXftBqA7Zh9oRDcq/hGAvqAjTn 5cHE10vKiHQdgRmFOe8uUvTnqG3cGLOBrFv8i+oKkrFn2Mz8NqZlspOB4Ks0V0tz BLA75hOoF4GoSY3t72OR/42+Mnf9ixEfOsLrH6uoYJiPEQwpXWji6aWc= X-Virus-Scanned: Debian amavisd-new at mail.nppct.ru Received: from mail.nppct.ru ([127.0.0.1]) by mail.nppct.ru (mail.nppct.ru [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id DMa2CIqljPHy for ; Thu, 17 Apr 2025 14:19:50 +0300 (MSK) Received: from [172.16.0.185] (unknown [176.59.174.214]) by mail.nppct.ru (Postfix) with ESMTPSA id 750791C08C3; Thu, 17 Apr 2025 14:19:37 +0300 (MSK) Message-ID: Date: Thu, 17 Apr 2025 14:19:36 +0300 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] xen-netfront: handle NULL returned by xdp_convert_buff_to_frame() To: =?UTF-8?B?SsO8cmdlbiBHcm/Dnw==?= , Jakub Kicinski Cc: Stefano Stabellini , Oleksandr Tyshchenko , "David S. Miller" , Eric Dumazet , Paolo Abeni , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , xen-devel@lists.xenproject.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, bpf@vger.kernel.org, lvc-project@linuxtesting.org, stable@vger.kernel.org References: <20250414183403.265943-1-sdl@nppct.ru> <20250416175835.687a5872@kernel.org> <0c29a3f9-9e22-4e44-892d-431f06555600@suse.com> <452bac2e-2840-4db7-bbf4-c41e94d437a8@nppct.ru> <8264519a-d58a-486e-b3c5-dba400658513@nppct.ru> <4679ca25-572b-44aa-bc00-cb9dc1c0080c@suse.com> Content-Language: en-US From: Alexey In-Reply-To: <4679ca25-572b-44aa-bc00-cb9dc1c0080c@suse.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 17.04.2025 13:23, Jürgen Groß wrote: > On 17.04.25 12:06, Alexey wrote: >> >> On 17.04.2025 11:51, Juergen Gross wrote: >>> On 17.04.25 10:45, Alexey wrote: >>>> >>>> On 17.04.2025 10:12, Jürgen Groß wrote: >>>>> On 17.04.25 09:00, Alexey wrote: >>>>>> >>>>>> On 17.04.2025 03:58, Jakub Kicinski wrote: >>>>>>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote: >>>>>>>>           get_page(pdata); >>>>>>> Please notice this get_page() here. >>>>>>> >>>>>>>>           xdpf = xdp_convert_buff_to_frame(xdp); >>>>>>>> +        if (unlikely(!xdpf)) { >>>>>>>> + trace_xdp_exception(queue->info->netdev, prog, act); >>>>>>>> +            break; >>>>>>>> +        } >>>>>> Do you mean that it would be better to move the get_page(pdata) >>>>>> call lower, >>>>>> after checking for NULL in xdpf, so that the reference count is >>>>>> only increased >>>>>> after a successful conversion? >>>>> >>>>> I think the error handling here is generally broken (or at least very >>>>> questionable). >>>>> >>>>> I suspect that in case of at least some errors the get_page() is >>>>> leaking >>>>> even without this new patch. >>>>> >>>>> In case I'm wrong a comment reasoning why there is no leak should be >>>>> added. >>>>> >>>>> >>>>> Juergen >>>> >>>> I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return() >>> >>> Agreed. But what if xennet_xdp_xmit() returns an error < 0? >>> >>> In this case xdp_return_frame_rx_napi() won't be called. >>> >>> >>> Juergen >> >> Agreed. There is no explicit freed pdata in the calling function >> xennet_get_responses(). Without this, the page referenced by pdata >> could be leaked. >> >> I suggest: > > Could you please merge the two if () blocks, as they share the > call of xdp_return_frame_rx_napi() now? Something like: > > if (unlikely(err <= 0)) { >     if (err < 0) >         trace_xdp_exception(queue->info->netdev, prog, act); >     xdp_return_frame_rx_napi(xdpf); > } > > Juergen > > P.S.: please don't use HTML in emails I can't do this because xennet_xdp_xmit() can return a value > 0