On Wed, Aug 02, 2023 at 09:53:54AM +0300, Dan Carpenter wrote:
> Hello Carlos Maiolino,
>
> The patch 9a9f8f590f6d: "shmem: quota support" from Jul 25, 2023
> (linux-next), leads to the following Smatch static checker warning:
>
> fs/quota/dquot.c:1271 flush_warnings()
> warn: sleeping in atomic context
>
Thanks for the report Dan!
> fs/quota/dquot.c
> 1261 static void flush_warnings(struct dquot_warn *warn)
> 1262 {
> 1263 int i;
> 1264
> 1265 for (i = 0; i < MAXQUOTAS; i++) {
> 1266 if (warn[i].w_type == QUOTA_NL_NOWARN)
> 1267 continue;
> 1268 #ifdef CONFIG_PRINT_QUOTA_WARNING
> 1269 print_warning(&warn[i]);
> 1270 #endif
> --> 1271 quota_send_warning(warn[i].w_dq_id,
> 1272 warn[i].w_sb->s_dev, warn[i].w_type);
>
> The quota_send_warning() function does GFP_NOFS allocations, which don't
> touch the fs but can still sleep. GFP_ATOMIC or GFP_NOWAIT don't sleep.
>
Hmm, tbh I think the simplest way to fix it is indeed change GFP_NOFS to
GFP_NOWAIT when calling genlmsg_new(), quota_send_warnings() already abstain to
pass any error back to its caller, I don't think moving it from GFP_NOFS ->
GFP_NOWAIT will have much impact here as the amount of memory required for it is
not that big and wouldn't fail unless free memory is really short. I might be
wrong though.
If not that, another option would be to swap tmpfs spinlocks for mutexes, but I
would rather avoid that.
CC'ing other folks for more suggestions.
Carlos
> 1273 }
> 1274 }
>
> The call trees that Smatch is worried about are listed. The "disables
> preempt" functions take the spin_lock_irq(&info->lock) before calling
> shmem_recalc_inode().
>
> shmem_charge() <- disables preempt
> shmem_uncharge() <- disables preempt
> shmem_undo_range() <- disables preempt
> shmem_getattr() <- disables preempt
> shmem_writepage() <- disables preempt
> shmem_set_folio_swapin_error() <- disables preempt
> shmem_swapin_folio() <- disables preempt
> shmem_get_folio_gfp() <- disables preempt
> -> shmem_recalc_inode()
> -> shmem_inode_unacct_blocks()
> -> dquot_free_block_nodirty()
> -> dquot_free_space_nodirty()
> -> __dquot_free_space()
> -> flush_warnings()
Hm, I see, we add dquot_free_block_nodirty() call to shmem_inode_unacct_blocks()
here, which leads to this possible sleep inside spin_lock_irq().
>
> regards,
> dan carpenter
