netdev - Re: [PATCH 1/2 net-next v3] igc: avoid kernel warning when changing RX ring parameters

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

Message-ID: <YeHOHp2vztm1Oi5V@calimero.vinschen.de>
Date:   Fri, 14 Jan 2022 20:25:18 +0100
From:   Corinna Vinschen <vinschen@...hat.com>
To:     Vinicius Costa Gomes <vinicius.gomes@...el.com>
Cc:     intel-wired-lan@...osl.org, netdev@...r.kernel.org,
        Lennert Buytenhek <buytenh@...tstofly.org>,
        Alexander Lobakin <alexandr.lobakin@...el.com>
Subject: Re: [PATCH 1/2 net-next v3] igc: avoid kernel warning when changing
 RX ring parameters

On Jan 14 11:10, Vinicius Costa Gomes wrote:
> Corinna Vinschen <vinschen@...hat.com> writes:
> 
> > Calling ethtool changing the RX ring parameters like this:
> >
> >   $ ethtool -G eth0 rx 1024
> >
> > on igc triggers the "Missing unregister, handled but fix driver" warning in
> > xdp_rxq_info_reg().
> >
> > igc_ethtool_set_ringparam() copies the igc_ring structure but neglects to
> > reset the xdp_rxq_info member before calling igc_setup_rx_resources().
> > This in turn calls xdp_rxq_info_reg() with an already registered xdp_rxq_info.
> >
> > Make sure to unregister the xdp_rxq_info structure first in
> > igc_setup_rx_resources.  Move xdp_rxq_info handling down to bethe last
> > action, thus allowing to remove the xdp_rxq_info_unreg call in the error path.
> >
> > Fixes: 73f1071c1d29 ("igc: Add support for XDP_TX action")
> > Signed-off-by: Corinna Vinschen <vinschen@...hat.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> > [...]
> > @@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
> >  	rx_ring->next_to_clean = 0;
> >  	rx_ring->next_to_use = 0;
> >  
> > +	/* XDP RX-queue info */
> > +	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
> > +		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
> > +	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> > +			       rx_ring->q_vector->napi.napi_id);
> > +	if (res < 0) {
> > +		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> > +			   index);
> > +		return res;
> 
> Here and in the igb patch, it should be 'goto err', no?

D'oh, of course.  Soory and thanks for catching.  I'll prepare a v4.

> Another suggestion is to add the warning that Lennert reported in the
> commit message (the comment from Maciej in that other thread).

The current commit message already mentiones the "Missing unregister,
handled but fix driver" warning.  Do you mean the entire warning
snippet including call stack?  If so, no problem.  I'll add it to v4,
too.

Shall I also add "Reported-by: Lennert ..."?  Funny enough we
encountered the problem independently at almost the same time, so when I
sent my v1 of the patch I wasn't even aware of the thread started by
Lennert and only saw it afterwards :}

> Apart from that, I think this is cleaner than what I had proposed.

Thanks,
Corinna