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

Date: Fri, 19 May 2023 21:30:40 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Manish Chopra <manishc@...vell.com>
Cc: <netdev@...r.kernel.org>, <aelior@...vell.com>, <palok@...vell.com>,
 Sudarsana Kalluru <skalluru@...vell.com>, David Miller
 <davem@...emloft.net>
Subject: Re: [PATCH v4 net] qede: Fix scheduling while atomic

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.

> +	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.

> +		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