Project

General

Profile

Bug #4088

logging: Add API to enable mutex protecting around llogging target list

Added by pespin almost 2 years ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
libosmocore
Target version:
-
Start date:
07/04/2019
Due date:
% Done:

100%

Spec Reference:

Description

From discussion in https://gerrit.osmocom.org/c/osmo-trx/+/14645:

why do we have a log mutex in the first place?  I did some research into multithreadded logging using libosmocore and found that the only critical part would be adding/removing of log targets at runtime (the linked list of log targets).  However, during the actual log output / log write, as long as you don't use LOGPC, IIRC you should be fine.  Every fprintf should be atomic, whether to stdout or to any other file.
  Hi, good point. I did some reseach and here are my conclusions:

* Agree, the only "really dangerous" part seems to be log targets. 

Some nuisances or weird situations of not using mutex:
* unordered timestamps on generated output (th1 generates timestamp, th2 preempts th1 generates timestamp and prints, th1 prints).
* color tag left opened if color is disabled through VTY (log color is disabled while other thread is in the middle of generating log line). 

So the important cases are basically a thread calling LOGPSRC while another calls:
* race conditons during llog_targets_reopen()->log_target_file_reopen()
* logging_vty.c "logging enable", "log syslog local", "log gsmtap", "log stderr", etc. -> log_add_target() -> append to the list osmo_log_target_list
* logging_vty.c "logging disable" -> log_del_target() -> osmo_log_target_list 

so I think all in all there should be a mutex. But actually the one in osmo-trx doesn't seem to be covering the VTY commands AFAICT, so it's really not useful against big issues (but it's useful against nuisances explained first).

The steps to procede should be IMHO:
* Keep the mutex for now in osmo-trx to at least solve nuisances.
* Add some sort of multithread support in libosmocore's logging system (by adding an internal mutex around log targets (and list), and enabling using the mutex through a libosmocore public API (such as log_enable_multithread_support(bool enable)).
* Once libosmocore has that multithread support, drop the osmo-trx mutex and use the libosmocore API. 

Related issues

Related to libosmocore - Bug #4089: libosmovty: ASan heap-use-after-free in osmo-trx while messing with VTY (debug enabled)Resolved07/04/2019

Associated revisions

Revision d12f698d (diff)
Added by pespin over 1 year ago

logging: Introduce mutex API to manage log_target in multi-thread envs

log_enable_multithread() enables use of locks inside the
implementation. Lock use is disabled by default, this way only
multi-thread processes need to enable it and suffer related
complexity/performance penalties.

Locks are required around osmo_log_target_list and items inside it,
since targets can be used, modified and deleted by different threads
concurrently (for instance, user writing "logging disable" in VTY while
another thread is willing to write into that target).

Multithread apps and libraries aiming at being used in multithread apps
should update their code to use the locks introduced here when
containing code iterating over osmo_log_target_list explictly or
implicitly by obtaining a log_target (eg. osmo_log_vty2tgt()).

Related: OS#4088
Change-Id: Id7711893b34263baacac6caf4d489467053131bb

Revision b7e99270 (diff)
Added by pespin over 1 year ago

Use new libosmocore logging lock API

Since libosmocore Id7711893b34263baacac6caf4d489467053131bb, a new API
log_enable_multithread() is available which takes care of protecting
logging infrastructure from us (and actually does it correctly since we
cannot protect internal libosmocore structures from osmo-trx).

Let's drop all mutex related code from osmo-trx logging infra and simply
rely on libosmocore's.

Related: OS#4088
Change-Id: I519d0f30bce871005ca26b90177ea4aa4839360a

History

#1 Updated by pespin over 1 year ago

libosmocore:
remote: https://gerrit.osmocom.org/c/libosmocore/+/15558 logging_internal.h: Fix osmo_log_info definition
remote: https://gerrit.osmocom.org/c/libosmocore/+/15559 logging: Move osmo_log_target_list from logging.h to logging_internal.h
remote: https://gerrit.osmocom.org/c/libosmocore/+/15560 logging: Introduce mutex API to manage log_target in multi-thread envs

osmo-trx:
https://gerrit.osmocom.org/c/osmo-trx/+/15561 Use new libosmocore logging lock API

#2 Updated by pespin over 1 year ago

  • Status changed from New to Feedback
  • % Done changed from 0 to 90

#3 Updated by pespin over 1 year ago

  • Related to Bug #4089: libosmovty: ASan heap-use-after-free in osmo-trx while messing with VTY (debug enabled) added

#4 Updated by pespin over 1 year ago

  • Status changed from Feedback to Resolved
  • % Done changed from 90 to 100

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)