netdev - RE: [EXT] Re: [PATCH v4 net] qede: Fix scheduling while atomic

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: 
 <BY3PR18MB46126D0DCC780CC494F45220AB439@BY3PR18MB4612.namprd18.prod.outlook.com>
Date: Mon, 22 May 2023 13:09:56 +0000
From: Manish Chopra <manishc@...vell.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ariel Elior
	<aelior@...vell.com>, Alok Prasad <palok@...vell.com>,
        Sudarsana Reddy
 Kalluru <skalluru@...vell.com>,
        David Miller <davem@...emloft.net>
Subject: RE: [EXT] Re: [PATCH v4 net] qede: Fix scheduling while atomic

> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Saturday, May 20, 2023 10:01 AM
> To: Manish Chopra <manishc@...vell.com>
> Cc: netdev@...r.kernel.org; Ariel Elior <aelior@...vell.com>; Alok Prasad
> <palok@...vell.com>; Sudarsana Reddy Kalluru <skalluru@...vell.com>;
> David Miller <davem@...emloft.net>
> Subject: [EXT] Re: [PATCH v4 net] qede: Fix scheduling while atomic
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Thu, 18 May 2023 20:22:14 +0530 Manish Chopra wrote:
> > Bonding module collects the statistics while holding the spinlock,
> > beneath that qede->qed driver statistics flow gets scheduled out due
> > to usleep_range() used in PTT acquire logic which results into below
> > bug and traces -
> 
> >  	struct qede_dump_info		dump_info;
> > +	struct delayed_work		periodic_task;
> > +	unsigned long			stats_coal_interval;
> > +	u32				stats_coal_ticks;
> 
> It's a bit odd to make _interval ulong and ticks u32 when _ticks will obviously
> be much larger..
> 
> Also - s/ticks/usecs/ ? I'd have guessed interval == usecs, ticks == jiffies
> without reading the code, and the inverse is true.

I will rename to avoid the confusion (ticks to usecs and interval to ticks).

> 
> > +	spinlock_t			stats_lock; /* lock for vport stats
> access */
> >  };
> 
> > +	if (edev->stats_coal_ticks != coal->stats_block_coalesce_usecs) {
> > +		u32 stats_coal_ticks, prev_stats_coal_ticks;
> > +
> > +		stats_coal_ticks = coal->stats_block_coalesce_usecs;
> > +		prev_stats_coal_ticks = edev->stats_coal_ticks;
> > +
> > +		/* zero coal ticks to disable periodic stats */
> > +		if (stats_coal_ticks)
> > +			stats_coal_ticks = clamp_t(u32, stats_coal_ticks,
> > +
> QEDE_MIN_STATS_COAL_TICKS,
> > +
> QEDE_MAX_STATS_COAL_TICKS);
> > +
> > +		stats_coal_ticks = rounddown(stats_coal_ticks,
> QEDE_MIN_STATS_COAL_TICKS);
> > +		edev->stats_coal_ticks = stats_coal_ticks;
> 
> Why round down the usecs?  Don't you want to return to the user on get
> exactly what set specified?  Otherwise I wouldn't bother saving the usecs at
> all, just convert back from jiffies.

Maybe I should not put this and allow user to configure any usecs value (by not limiting to any min/max)
as long as it results into different/unique ticks/jiffies. But I should still save usecs rather than ticks/jiffies
(or preferably both in order to avoid runtime conversion at the time of scheduling the work every time)
because on get jiffies_to_usecs() could be misleading, for example if user configured 10 usecs which will
result into single jiffy and on getting it back it will report 1000 usecs when converting back using jiffies_to_usecs()
which is still not the same as what user configured.

> 
> > +		if (edev->stats_coal_ticks) {
> > +			edev->stats_coal_interval = (unsigned long)edev-
> >stats_coal_ticks *
> > +							HZ / 1000000;
> 
> usecs_to_jiffies()
> 
> > +			if (prev_stats_coal_ticks == 0)
> > +				schedule_delayed_work(&edev-
> >periodic_task, 0);
> > +		}
> > +
> > +		DP_VERBOSE(edev, QED_MSG_DEBUG, "stats coal
> interval=%lu jiffies\n",
> > +			   edev->stats_coal_interval);
> > +	}
> > +
> >  	if (!netif_running(dev)) {
> >  		DP_INFO(edev, "Interface is down\n");
> >  		return -EINVAL;
> --
> pw-bot: cr