linux-kernel - Re: [PATCH net-next v3 5/6] netconsole: resume previously deactivated target

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <j67rta6sn3c2tgor3gtcrr2hvcdnxk6iqvzkhqkjkr6cgaezbh@vri4vhhzv5rf>
Date: Wed, 12 Nov 2025 09:52:10 -0800
From: Breno Leitao <leitao@...ian.org>
To: Andre Carvalho <asantostc@...il.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>, 
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v3 5/6] netconsole: resume previously
 deactivated target

On Tue, Nov 11, 2025 at 07:18:46PM +0000, Andre Carvalho wrote:
> On Tue, Nov 11, 2025 at 02:12:26AM -0800, Breno Leitao wrote:
> > > + *		disabled. Internally, although both STATE_DISABLED and
> > > + *		STATE_DEACTIVATED correspond to inactive netpoll the latter is>
> > > + *		due to interface state changes and may recover automatically.
> > 
> >  *		disabled. Internally, although both STATE_DISABLED and
> >  *		STATE_DEACTIVATED correspond to inactive targets, the latter is
> >  *		due to automatic interface state changes and will try
> >  *		recover automatically, if the interface comes back
> >  *		online.
> > 
> 
> This is much clearer, thanks for the suggestion. 
> 
> > > +	ret = __netpoll_setup_hold(&nt->np, ndev);
> > > +	if (ret) {
> > > +		/* netpoll fails setup once, do not try again. */
> > > +		nt->state = STATE_DISABLED;
> > > +	} else {
> > > +		nt->state = STATE_ENABLED;
> > > +		pr_info("network logging resumed on interface %s\n",
> > > +			nt->np.dev_name);
> > > +	}
> > > +}
> > 
> > I am not sure that helper is useful, I would simplify the last patch
> > with this one and write something like:
> > 
> 
> The main reason why I opted for a helper in netpoll was to keep reference
> tracking for these devices strictly inside netpoll and have simmetry between
> setup and cleanup. Having said that, this might be an overkill and I'm fine with 
> dropping the helper and taking your suggestion.

Right, that makes sense. Would we have other owners for that function?

> 
> > > +
> > > +/* Check if the target was bound by mac address. */
> > > +static bool bound_by_mac(struct netconsole_target *nt)
> > > +{
> > > +	return is_valid_ether_addr(nt->np.dev_mac);
> > > +}
> > 
> > Awesome. I liked this helper. It might be useful it some other places, and
> > eventually transformed into a specific type in the target (in case we need to
> > in the future)
> > 
> > Can we use it egress_dev also? If so, please separate this in a separate patch.
> 
> In order to do that, we'd need to move bound_by_mac to netpolland make it available
> to be called by netconsole. Let me know if you'd like me to do this in this series,
> otherwise I'm also happy to refactor this separately from this series.

Oh, I see the problem. That egress_dev() should belong to netconsole not
netpoll.

I've sent a patchset to start untangling netconsole and netpoll, and the
patchset was conflicting with the fix in 'net' 

https://lore.kernel.org/all/20250902-netpoll_untangle_v3-v1-0-51a03d6411be@debian.org/

Let's keep egress_dev() as it is for now, until we got them untangled.

> 
> > > +		if (nt->state == STATE_DEACTIVATED && event == NETDEV_UP &&
> > > +		    target_match(nt, dev))
> > > +			list_move(&nt->list, &resume_list);
> > 
> > I think it would be better to move the nt->state == STATE_DEACTIVATED to target_match and use
> > the case above. As the following:
> > 
> > 	if (nt->np.dev == dev) {
> > 		switch (event) {
> > 		case NETDEV_CHANGENAME:
> > 		....
> > 		case NETDEV_UP:
> > 			if (target_match(nt, dev))
> > 				list_move(&nt->list, &resume_list);
> > 
> 
> We are not able to handle this inside this switch because when target got deactivated, 

You are right, that is why we are doing the magic here. Please add
a comment in saying that maybe_resume_target() is IRQ usafe, thus,
cannot be called with IRQ disabled.

> do_netpoll_cleanup sets nt->np.dev = NULL. Having said that, I can still move nt->state == STATE_DEACTIVATED
> to inside target_match (maybe calling it deactivated_target_match) to make this slightly more readable. 

Awesome. Thanks for the patch!
--breno