Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rasdaemon: Fix for regression in ras_mc_create_table() if some cpus a… #93

Closed
wants to merge 1 commit into from

Conversation

shijujose4
Copy link
Contributor

…re offline at the system start

Issues:
Regression in the ras_mc_create_table() if some of the cpus are offline at the system start when run the rasdaemon. This issue is reproducible in ras_mc_create_table() with decode and record non-standard events and reproducible sometimes with ras_mc_create_table() for the standard events.
Also in the multi thread way, there is memory leak in ras_mc_event_opendb() as struct sqlite3_priv *priv and sqlite3 *db allocated/initialized per thread, but stored in the common struct ras_events ras in pthread data, which is shared across the threads.

Reason:
when the system start with some of the cpus are offline and then run the rasdaemon, read_ras_event_all_cpus() exit with error and switch to the multi thread way. However read() in read_ras_event() return error in threads for each of the offline CPUs and does clean up including calling ras_mc_event_closedb().
Since the 'struct ras_events ras' passed in the pthread_data to each of the threads is common, struct sqlite3_priv *priv and sqlite3 *db allocated/ initialized per thread and stored in the common 'struct ras_events ras', are getting overwritten in each ras_mc_event_opendb()(which called from pthread per cpu), result memory leak. Also when ras_mc_event_closedb() is called in the above error case from the threads corresponding to the offline cpus, close the sqlite3 *db and free sqlite3_priv *priv stored in the common 'struct ras_events ras', result regression when accessing priv->db in the ras_mc_create_table() from another context later.

Proposed solution:
In ras_mc_event_opendb(), allocate struct sqlite3_priv *priv, init sqlite3 *db and create tables common for the threads with shared 'struct ras_events ras' based on a reference count and free them in the same way.
Also protect critical code ras_mc_event_opendb() and ras_mc_event_closedb() using mutex in the multi thread case from any regression caused by the thread pre-emption.

Reported-by: Lei Feng fenglei47@h-partners.com

@thesamesam
Copy link
Contributor

This sounds like it could be what we're hitting in https://bugs.gentoo.org/890286 and #77.

@mchehab we're getting quite a few users hitting this in Gentoo. Does this look right? (I'm wondering if we should apply it downstream.)

…re offline at the system start

Issues:
Regression in the ras_mc_create_table() if some of the cpus are offline
at the system start when run the rasdaemon. This issue is
reproducible in ras_mc_create_table() with decode and record
non-standard events and reproducible sometimes with
ras_mc_create_table() for the standard events.
Also in the multi thread way, there is memory leak in ras_mc_event_opendb()
as struct sqlite3_priv *priv and sqlite3 *db allocated/initialized per
thread, but stored in the common struct ras_events ras in pthread data,
which is shared across the threads.

Reason:
when the system start with some of the cpus are offline and then run
the rasdaemon, read_ras_event_all_cpus() exit with error and switch to
the multi thread way. However read() in read_ras_event() return error in
threads for each of the offline CPUs and does clean up including calling
ras_mc_event_closedb().
Since the 'struct ras_events ras' passed in the pthread_data to each of the
threads is common, struct sqlite3_priv *priv and sqlite3 *db allocated/
initialized per thread and stored in the common 'struct ras_events ras',
are getting overwritten in each ras_mc_event_opendb()(which called from
pthread per cpu), result memory leak. Also when ras_mc_event_closedb()
is called in the above error case from the threads corresponding to the
offline cpus, close the sqlite3 *db and free sqlite3_priv *priv stored
in the common 'struct ras_events ras', result regression when accessing
priv->db in the ras_mc_create_table() from another context later.

Proposed solution:
In ras_mc_event_opendb(), allocate struct sqlite3_priv *priv,
init sqlite3 *db and create tables common for the threads with shared
'struct ras_events ras' based on a reference count and free them in the
same way.
Also protect critical code ras_mc_event_opendb() and ras_mc_event_closedb()
using mutex in the multi thread case from any regression caused by the
thread pre-emption.

Reported-by: Lei Feng <fenglei47@h-partners.com>
Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
@mchehab
Copy link
Owner

mchehab commented Apr 30, 2023

Applied, thanks!

@mchehab mchehab closed this Apr 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants