netdev - Re: [PATCH] sky2: Fix a race condition in sky2

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:	Mon, 6 Jul 2009 11:51:01 -0700
From:	Stephen Hemminger <shemminger@...ux-foundation.org>
To:	Mike McCormack <mikem@...g3k.org>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH] sky2: Fix a race condition in sky2_poll

On Sat, 20 Jun 2009 17:01:25 +0900
Mike McCormack <mikem@...g3k.org> wrote:

> Clear interrupt only when the status buffer is fully drained,
> Make sure to clear interrupt when work_done == work_limit
>  and the buffer is drained.
> ---
>  drivers/net/sky2.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
> index 7681d28..ca1e9e5 100644
> --- a/drivers/net/sky2.c
> +++ b/drivers/net/sky2.c
> @@ -2524,9 +2524,6 @@ static int sky2_status_intr(struct sky2_hw *hw,
> int to_do, u16 idx)
>  		}
>  	} while (hw->st_idx != idx);
> 
> -	/* Fully processed status ring so clear irq */
> -	sky2_write32(hw, STAT_CTRL, SC_STAT_CLR_IRQ);
> -
>  exit_loop:
>  	sky2_rx_done(hw, 0, total_packets[0], total_bytes[0]);
>  	sky2_rx_done(hw, 1, total_packets[1], total_bytes[1]);
> @@ -2779,9 +2776,16 @@ static int sky2_poll(struct napi_struct *napi,
> int work_limit)
>  	if (status & Y2_IS_IRQ_PHY2)
>  		sky2_phy_intr(hw, 1);
> 
> -	while ((idx = sky2_read16(hw, STAT_PUT_IDX)) != hw->st_idx) {
> +	idx = sky2_read16(hw, STAT_PUT_IDX);
> +	while (idx != hw->st_idx) {
>  		work_done += sky2_status_intr(hw, work_limit - work_done, idx);
> 
> +		/* If we fully processed the status ring, clear the irq */
> +		idx = sky2_read16(hw, STAT_PUT_IDX);
> +		if (idx == hw->st_idx) {
> +			sky2_write32(hw, STAT_CTRL, SC_STAT_CLR_IRQ);
> +			break;
> +		}
>  		if (work_done >= work_limit)
>  			goto done;
>  	}

Have you actually seen this race, or are you hypothesizing based
on code review?


I think the original works fine. There is a race where interrupt is cleared early,
and the poll processing runs an extra time but that is harmless.

But the patched code races the other way:

> +	while (idx != hw->st_idx) {
>  		work_done += sky2_status_intr(hw, work_limit - work_done, idx);
> 
> +		/* If we fully processed the status ring, clear the irq */
> +		idx = sky2_read16(hw, STAT_PUT_IDX);

Packet arrives right here. The variable "idx" has old value,
but chip register "put_idx" shows new packet.

> +		if (idx == hw->st_idx) {
> +			sky2_write32(hw, STAT_CTRL, SC_STAT_CLR_IRQ);

This clears level triggered status interrupt.

> +			break;
> +		}
>  		if (work_done >= work_limit)
>  			goto done;
>  	}

Now the driver misses the status interrupt.



-- 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html