netdev - Re: [PATCH net] sfc: remove ASSERT_RTNL() from get_ts

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] [day] [month] [year] [list]

Message-ID: <dbe283c0-76ea-46ca-b06d-9925b16d2a69@redhat.com>
Date: Mon, 25 Aug 2025 23:12:46 +0300
From: mohammad heib <mheib@...hat.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, ecree.xilinx@...il.com
Subject: Re: [PATCH net] sfc: remove ASSERT_RTNL() from get_ts_info function

Hi Andrew,

Thanks for the feedback.
My initial version was actually to add rtnl_lock inside 
sock_set_timestamping since that function was added
by this patch:
https://patchwork.kernel.org/project/netdevbpf/patch/20210630081202.4423-9-yangbo.lu@nxp.com/
without considering RTNL and the safety it provides.

I'm not that experienced with locking, and adding RTNL there felt like a 
risky change to me even though it's
a control path so I was a bit hesitant to do it.

That's why I decided instead to send this patch that removes the 
assertion, mainly to get feedback and a heads-up
from people with more experience in this area.

if you think it will be safe to add rtnl lock inside the 
sock_set_timestamping i can rework my change.

On 8/25/25 7:25 PM, Andrew Lunn wrote:
> On Mon, Aug 25, 2025 at 04:57:49PM +0300, mheib@...hat.com wrote:
>> From: Mohammad Heib <mheib@...hat.com>
>>
>> The SFC driver currently asserts that the RTNL lock is held in
>> efx_ptp_get_ts_info() using ASSERT_RTNL(). While this is correct for
>> the ethtool ioctl path, this function can also be called from the
>> SO_TIMESTAMPING socket path where RTNL is not held, which triggers
>> kernel BUGs in debug builds.
>>
>> This patch removes the ASSERT_RTNL() to avoid these assertions in
>> kernel logs when called from paths that do not hold RTNL.
> What is missing from the commit message is an explanation of why RTNL
> does not need to be held in this function. Maybe this is a real bug
> and you are just hiding it, rather than fixing it?
>
>      Andrew
>