netdev - Re: [PATCHv3 net-next] bonding: add software tx timestamping support

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: <20230412072555.38c7288f@kernel.org>
Date:   Wed, 12 Apr 2023 07:25:55 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     Jay Vosburgh <jay.vosburgh@...onical.com>, netdev@...r.kernel.org,
        "David S . Miller" <davem@...emloft.net>,
        Jonathan Toppins <jtoppins@...hat.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Liang Li <liali@...hat.com>,
        Simon Horman <simon.horman@...igine.com>,
        Miroslav Lichvar <mlichvar@...hat.com>
Subject: Re: [PATCHv3 net-next] bonding: add software tx timestamping
 support

On Wed, 12 Apr 2023 20:28:08 +0800 Hangbin Liu wrote:
> > 	Ok, maybe I didn't look at that carefully enough, and now that I
> > do, it's really complicated.
> > 
> > 	Going through it, I think the call path that's relevant is
> > taprio_change -> taprio_parse_clockid -> ethtool_ops->get_ts_info.
> > taprio_change is Qdisc_ops.change function, and tc_modify_qdisc should
> > come in with RTNL held.
> > 
> > 	If I'm reading cscope right, the other possible caller of
> > Qdisc_ops.change is fifo_set_limit, and it looks like that function is
> > only called by functions that are themselves Qdisc_ops.change functions
> > (red_change -> __red_change, sfb_change, tbf_change) or Qdisc_ops.init
> > functions (red_init -> __red_change, sfb_init, tbf_init).
> > 
> > 	There's also a qdisc_create_dflt -> Qdisc_ops.init call path,
> > but I don't know if literally all calls to qdisc_create_dflt hold RTNL.
> > 
> > 	There's a lot of them, and I'm not sure how many of those could
> > ever end up calling into taprio_change (if, say, a taprio qdisc is
> > attached within another qdisc).
> > 
> > 	qdisc_create also calls Qdisc_ops.init, but that one seems to
> > clearly expect to enter with RTNL.
> > 
> > 	Any tc expert able to state for sure whether it's possible to
> > get into any of the above without RTNL?  I suspect it isn't, but I'm not
> > 100% sure either.  
> 
> You dug more than me. Maybe we can add an ASSERT_RTNL() checking here first?
> But since we can't 100% sure we are holding the rtnl lock, I think we
> can keep the rcu lock for safe. I saw rlb_next_rx_slave() also did the same...

ASSERT_RTNL sounds good. I think that drivers may expect rtnl lock to 
be held around ethtool ops, so if some path is not holding it - I'd
count that as a bug.

> > >You could check in this loop if TX is supported...  
> > 
> > 	I see your point below about not wanting to create
> > SOFT_TIMESTAMPING_SOFTRXTX, but doesn't the logic need to test all three
> > of the flags _TX_SOFTWARE, _RX_SOFTWARE, and _SOFTWARE?  
> 
> I think Jakub means we have already add _RX_SOFTWARE and _SOFTWARE for bonding
> whatever slave's flag, then we just need to check slave's _TX_SOFTWARE flag.

Indeed.