| Message ID | 20230508075812.76077-5-eiichi.tsukata@nutanix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | audit: refactor and fix for potential deadlock | expand |
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/202305090112.uJSc0NBw-lkp@intel.com/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 <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305090112.uJSc0NBw-lkp@intel.com/
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
> On May 9, 2023, at 2:13, kernel test robot <lkp@intel.com> 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 <lkp@intel.com> > | 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
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;
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 <eiichi.tsukata@nutanix.com> --- kernel/audit.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)