Commit 7ffb8e317bae ("audit: we don't need to
__set_current_state(TASK_RUNNING)") accidentally moved queue full check
before add_wait_queue_exclusive() which introduced the following race:
CPU1 CPU2
======== ========
(in audit_log_start()) (in kauditd_thread())
queue is full
wake_up(&audit_backlog_wait)
wait_event_freezable()
add_wait_queue_exclusive()
...
schedule_timeout()
Once this happens, both audit_log_start() and kauditd_thread() can cause
deadlock for up to backlog_wait_time waiting for each other. To prevent
the race, this patch adds queue full check after
prepare_to_wait_exclusive().
Fixes: 7ffb8e317bae ("audit: we don't need to __set_current_state(TASK_RUNNING)")
Signed-off-by: Eiichi Tsukata <[email protected]>
---
kernel/audit.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index bcbb0ba33c84..d37a3a045230 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -644,8 +644,14 @@ static long wait_for_kauditd(long stime)
prepare_to_wait_exclusive(&audit_backlog_wait, &wait,
TASK_UNINTERRUPTIBLE);
- rtime = schedule_timeout(stime);
- atomic_add(stime - rtime, &audit_backlog_wait_time_actual);
+
+ /* need to check if the queue is full again because kauditd might have
+ * flushed the queue and went to sleep after prepare_to_wait_exclusive()
+ */
+ if (audit_queue_full(&audit_queue)) {
+ rtime = schedule_timeout(stime);
+ atomic_add(stime - rtime, &audit_backlog_wait_time_actual);
+ }
finish_wait(&audit_backlog_wait, &wait);
return rtime;
--
2.40.0
Hi Eiichi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on pcmoore-audit/next]
[also build test WARNING on linus/master v6.4-rc1 next-20230508]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Eiichi-Tsukata/audit-refactor-queue-full-checks/20230508-160019
base: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git next
patch link: https://lore.kernel.org/r/20230508075812.76077-5-eiichi.tsukata%40nutanix.com
patch subject: [PATCH 4/4] audit: check if queue is full after prepare_to_wait_exclusive()
config: i386-randconfig-a001-20230508 (https://download.01.org/0day-ci/archive/20230509/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/cbc69d0c34bdbc06ebca3e3020cfc24034fcf173
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Eiichi-Tsukata/audit-refactor-queue-full-checks/20230508-160019
git checkout cbc69d0c34bdbc06ebca3e3020cfc24034fcf173
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> kernel/audit.c:651:6: warning: variable 'rtime' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (audit_queue_full(&audit_queue)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/audit.c:657:9: note: uninitialized use occurs here
return rtime;
^~~~~
kernel/audit.c:651:2: note: remove the 'if' if its condition is always true
if (audit_queue_full(&audit_queue)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/audit.c:642:12: note: initialize the variable 'rtime' to silence this warning
long rtime;
^
= 0
1 warning generated.
vim +651 kernel/audit.c
630
631 /**
632 * wait_for_kauditd - Wait for kauditd to drain the queue
633 * @stime: time to sleep
634 *
635 * Description:
636 * Waits for kauditd to drain the queue then adds duration of sleep time to
637 * audit_backlog_wait_time_actual as cumulative sum.
638 * Returns remaining time to sleep.
639 */
640 static long wait_for_kauditd(long stime)
641 {
642 long rtime;
643 DEFINE_WAIT(wait);
644
645 prepare_to_wait_exclusive(&audit_backlog_wait, &wait,
646 TASK_UNINTERRUPTIBLE);
647
648 /* need to check if the queue is full again because kauditd might have
649 * flushed the queue and went to sleep after prepare_to_wait_exclusive()
650 */
> 651 if (audit_queue_full(&audit_queue)) {
652 rtime = schedule_timeout(stime);
653 atomic_add(stime - rtime, &audit_backlog_wait_time_actual);
654 }
655 finish_wait(&audit_backlog_wait, &wait);
656
657 return rtime;
658 }
659
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
> On May 9, 2023, at 2:13, kernel test robot <[email protected]> wrote:
>
> Hi Eiichi,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on pcmoore-audit/next]
> [also build test WARNING on linus/master v6.4-rc1 next-20230508]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://urldefense.proofpoint.com/v2/url?u=https-3A__git-2Dscm.com_docs_git-2Dformat-2Dpatch-23-5Fbase-5Ftree-5Finformation&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=dy01Dr4Ly8mhvnUdx1pZhhT1bkq4h9z5aVWu3paoZtk&m=iEsfAln6T0-ALFZ77M_oT-wS2T2UFxboG589BXhqJFYG7-pvNIFPZ_GOAcQLlAj7&s=hJmkWbARWhiKbVA6rbRJyTnRSNu6CCcbc8h_5kv9OMs&e= ]
>
> url: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_intel-2Dlab-2Dlkp_linux_commits_Eiichi-2DTsukata_audit-2Drefactor-2Dqueue-2Dfull-2Dchecks_20230508-2D160019&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=dy01Dr4Ly8mhvnUdx1pZhhT1bkq4h9z5aVWu3paoZtk&m=iEsfAln6T0-ALFZ77M_oT-wS2T2UFxboG589BXhqJFYG7-pvNIFPZ_GOAcQLlAj7&s=SgsiWqTcS-KNlz7pbWtpJLkfU4NCNx1PPnf561OBl_Y&e=
> base: https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_pcmoore_audit.git&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=dy01Dr4Ly8mhvnUdx1pZhhT1bkq4h9z5aVWu3paoZtk&m=iEsfAln6T0-ALFZ77M_oT-wS2T2UFxboG589BXhqJFYG7-pvNIFPZ_GOAcQLlAj7&s=ODnLSauQV_XYtGzJFN1XMH7WQhUdZfa4kgzeiRdieOk&e= next
> patch link: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_r_20230508075812.76077-2D5-2Deiichi.tsukata-2540nutanix.com&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=dy01Dr4Ly8mhvnUdx1pZhhT1bkq4h9z5aVWu3paoZtk&m=iEsfAln6T0-ALFZ77M_oT-wS2T2UFxboG589BXhqJFYG7-pvNIFPZ_GOAcQLlAj7&s=xz0pVOvfxWhxF6yp8r3GFk7YmDc34ntJLAvFhHkVWqo&e=
> patch subject: [PATCH 4/4] audit: check if queue is full after prepare_to_wait_exclusive()
> config: i386-randconfig-a001-20230508 (https://urldefense.proofpoint.com/v2/url?u=https-3A__download.01.org_0day-2Dci_archive_20230509_202305090112.uJSc0NBw-2Dlkp-40intel.com_config&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=dy01Dr4Ly8mhvnUdx1pZhhT1bkq4h9z5aVWu3paoZtk&m=iEsfAln6T0-ALFZ77M_oT-wS2T2UFxboG589BXhqJFYG7-pvNIFPZ_GOAcQLlAj7&s=q5366dSfHPWCbkUzXNPdIjJ8BOj2forT0D4fl3XwOQM&e= )
> compiler: clang version 14.0.6 (https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_llvm_llvm-2Dproject&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=dy01Dr4Ly8mhvnUdx1pZhhT1bkq4h9z5aVWu3paoZtk&m=iEsfAln6T0-ALFZ77M_oT-wS2T2UFxboG589BXhqJFYG7-pvNIFPZ_GOAcQLlAj7&s=cVl2Fv0ruc2lDCYKGZ2MsG1xkcmDSm0yVdz_UYbBk2A&e= f28c006a5895fc0e329fe15fead81e37457cb1d1)
> reproduce (this is a W=1 build):
> wget https://urldefense.proofpoint.com/v2/url?u=https-3A__raw.githubusercontent.com_intel_lkp-2Dtests_master_sbin_make.cross&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=dy01Dr4Ly8mhvnUdx1pZhhT1bkq4h9z5aVWu3paoZtk&m=iEsfAln6T0-ALFZ77M_oT-wS2T2UFxboG589BXhqJFYG7-pvNIFPZ_GOAcQLlAj7&s=Uwk-WxemwAN0EiTlBE5VBdk5GGUUOMySKGZXEEpkDBM&e= -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_intel-2Dlab-2Dlkp_linux_commit_cbc69d0c34bdbc06ebca3e3020cfc24034fcf173&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=dy01Dr4Ly8mhvnUdx1pZhhT1bkq4h9z5aVWu3paoZtk&m=iEsfAln6T0-ALFZ77M_oT-wS2T2UFxboG589BXhqJFYG7-pvNIFPZ_GOAcQLlAj7&s=RWanBTSRB28iteIUKeSVppj2xq4uJygfumsMWjJv5jI&e=
> git remote add linux-review https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_intel-2Dlab-2Dlkp_linux&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=dy01Dr4Ly8mhvnUdx1pZhhT1bkq4h9z5aVWu3paoZtk&m=iEsfAln6T0-ALFZ77M_oT-wS2T2UFxboG589BXhqJFYG7-pvNIFPZ_GOAcQLlAj7&s=14mmQ3GZhIozHeufPooQIuKd1O2SeFL8-1gIi9c79EM&e=
> git fetch --no-tags linux-review Eiichi-Tsukata/audit-refactor-queue-full-checks/20230508-160019
> git checkout cbc69d0c34bdbc06ebca3e3020cfc24034fcf173
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
> | Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_oe-2Dkbuild-2Dall_202305090112.uJSc0NBw-2Dlkp-40intel.com_&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=dy01Dr4Ly8mhvnUdx1pZhhT1bkq4h9z5aVWu3paoZtk&m=iEsfAln6T0-ALFZ77M_oT-wS2T2UFxboG589BXhqJFYG7-pvNIFPZ_GOAcQLlAj7&s=cKrS8zKrWpbCBg5KOGiZDk0iqjUu9BXkChKAu4pMC2Q&e=
>
> All warnings (new ones prefixed by >>):
>
>>> kernel/audit.c:651:6: warning: variable 'rtime' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> if (audit_queue_full(&audit_queue)) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> kernel/audit.c:657:9: note: uninitialized use occurs here
> return rtime;
> ^~~~~
> kernel/audit.c:651:2: note: remove the 'if' if its condition is always true
> if (audit_queue_full(&audit_queue)) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> kernel/audit.c:642:12: note: initialize the variable 'rtime' to silence this warning
> long rtime;
> ^
> = 0
> 1 warning generated.
>
Nice catch thanks!
We can initialize rtime as you suggested but in this case it will be better to return 0
explicitly if there is some room in the queue. i.e:
diff --git a/kernel/audit.c b/kernel/audit.c
index d37a3a045230..6b0cc0459984 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -651,6 +651,8 @@ static long wait_for_kauditd(long stime)
if (audit_queue_full(&audit_queue)) {
rtime = schedule_timeout(stime);
atomic_add(stime - rtime, &audit_backlog_wait_time_actual);
+ } else {
+ rtime = 0;
}
finish_wait(&audit_backlog_wait, &wait);
I’ll fix it in v2.
Eiichi