From: Shay Agroskin <email@example.com>
To: Jakub Kicinski <firstname.lastname@example.org>
Cc: <email@example.com>, <firstname.lastname@example.org>,
<email@example.com>, <firstname.lastname@example.org>, <email@example.com>,
<firstname.lastname@example.org>, <email@example.com>, <firstname.lastname@example.org>,
<email@example.com>, <firstname.lastname@example.org>, <email@example.com>,
<firstname.lastname@example.org>, <email@example.com>, <firstname.lastname@example.org>,
Subject: Re: [PATCH V1 net 1/3] net: ena: Prevent reset after device destruction
Date: Thu, 13 Aug 2020 15:51:46 +0300 [thread overview]
Message-ID: <email@example.com> (raw)
Jakub Kicinski <firstname.lastname@example.org> writes:
> On Wed, 12 Aug 2020 13:10:57 +0300 Shay Agroskin wrote:
>> This patch also removes the destruction of the timer and reset
>> from ena_remove() since the timer is destroyed by the
>> routine and the reset work is handled by this patch.
> You'd still have a use after free if the work runs after the
> device is
> removed. I think cancel_work_sync() gotta stay.
Hi, thank you for reviewing the patch. Short answer: I verified
that the ENA_FLAG_TRIGGER_RESET flag cannot be set after
ena_destroy_device() finishes its execution.
The ena_destroy_device() function is called with rtnl_lock() held,
so it cannot run in parallel with the reset function. Also the
destroy function clears the bit ENA_FLAG_TRIGGER_RESET without
which the reset function just exits without doing anything.
A problem can then only happen when some routine sets the
ENA_FLAG_TRIGGER_RESET bit before the reset function is executed,
the following describes all functions from which this bit can be
- check_* functions: these function are called from the timer
routine which is destroyed in ena_destroy_device(), so by the
time the rtnl_lock() released, the bit is cleared
- napi related functions (io_poll, xdp_io_poll, validate_rx_req_id
etc.): the napi is de-registered in ena_destroy_device(), so
none of these functions is called after destroying the device.
- xmit functions (ena_xmit_common, ena_tx_timeout): the device is
brought down and all its RX/TX resources are freed before
releasing the lock.
These are all the occurrences I found. Without this bit set, the
reset function would fail the 'if' check in this patch, and exit
without doing anything. Destroying the reset function explicitly
won't help since by the time we do it, the function can already be
next prev parent reply other threads:[~2020-08-13 12:52 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-12 10:10 [PATCH V1 net 0/3] Bug fixes for ENA ethernet driver Shay Agroskin
2020-08-12 10:10 ` [PATCH V1 net 1/3] net: ena: Prevent reset after device destruction Shay Agroskin
2020-08-12 17:52 ` Jakub Kicinski
2020-08-13 12:51 ` Shay Agroskin [this message]
2020-08-13 20:41 ` Jakub Kicinski
2020-08-16 10:25 ` Shay Agroskin
2020-08-12 10:10 ` [PATCH V1 net 2/3] net: ena: Change WARN_ON expression in ena_del_napi_in_range() Shay Agroskin
2020-08-12 10:10 ` [PATCH V1 net 3/3] net: ena: Make missed_tx stat incremental Shay Agroskin
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).