linux-kernel - Re: [PATCH] quota: don't let mark_dquot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4a4e6e08-3d4f-4e40-8bed-43aad2013b92@kernel.org>
Date: Thu, 11 Apr 2024 18:27:40 +0800
From: Chao Yu <chao@...nel.org>
To: Jan Kara <jack@...e.cz>
Cc: jack@...e.com, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] quota: don't let mark_dquot_dirty() fail silently

On 2024/4/8 22:30, Jan Kara wrote:
> On Sun 07-04-24 15:31:28, Chao Yu wrote:
>> mark_dquot_dirty() will callback to specified filesystem function,
>> it may fail due to any reasons, however, no caller will check return
>> value of mark_dquot_dirty(), so, it may fail silently, let's print
>> one line message for such case.
>>
>> Signed-off-by: Chao Yu <chao@...nel.org>
>> ---
>>   fs/quota/dquot.c | 23 +++++++++++++----------
>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
>> index dacbee455c03..c5df7863942a 100644
>> --- a/fs/quota/dquot.c
>> +++ b/fs/quota/dquot.c
>> @@ -399,21 +399,20 @@ int dquot_mark_dquot_dirty(struct dquot *dquot)
>>   EXPORT_SYMBOL(dquot_mark_dquot_dirty);
>>   
>>   /* Dirtify all the dquots - this can block when journalling */
>> -static inline int mark_all_dquot_dirty(struct dquot __rcu * const *dquots)
>> +static inline void mark_all_dquot_dirty(struct dquot __rcu * const *dquots)
>>   {
>> -	int ret, err, cnt;
>> +	int ret, cnt;
>>   	struct dquot *dquot;
>>   
>> -	ret = err = 0;
>>   	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
>>   		dquot = srcu_dereference(dquots[cnt], &dquot_srcu);
>> -		if (dquot)
>> -			/* Even in case of error we have to continue */
>> -			ret = mark_dquot_dirty(dquot);
>> -		if (!err)
>> -			err = ret;
>> +		if (!dquot)
>> +			continue;
>> +		ret = mark_dquot_dirty(dquot);
>> +		if (ret < 0)
>> +			quota_error(dquot->dq_sb,
>> +				"mark_all_dquot_dirty fails, ret: %d", ret);
> 
> Do you have any practical case you care about? Because in practice the

Actually, no.

> filesystem will usually report if there's some catastrophic error (and the
> errors from ->mark_dirty() all mean the filesystem is in unhealthy state).
> So this message just adds to the noise in the error log - and e.g. if the
> disk goes bad so we cannot write, we could spew a lot of messages like
> this.

Agreed,

I guess we can propagate the error to caller rather than printing redundant
message in log.

> 
>>   	}
>> -	return err;
>>   }
>>   
>>   static inline void dqput_all(struct dquot **dquot)
>> @@ -2725,6 +2724,7 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di)
>>   {
>>   	struct mem_dqblk *dm = &dquot->dq_dqb;
>>   	int check_blim = 0, check_ilim = 0;
>> +	int ret;
>>   	struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_id.type];
>>   
>>   	if (di->d_fieldmask & ~VFS_QC_MASK)
>> @@ -2807,7 +2807,10 @@ static int do_set_dqblk(struct dquot *dquot, struct qc_dqblk *di)
>>   	else
>>   		set_bit(DQ_FAKE_B, &dquot->dq_flags);
>>   	spin_unlock(&dquot->dq_dqb_lock);
>> -	mark_dquot_dirty(dquot);
>> +	ret = mark_dquot_dirty(dquot);
>> +	if (ret < 0)
>> +		quota_error(dquot->dq_sb,
>> +			"mark_dquot_dirty fails, ret: %d", ret);
> 
> Here, we can propagate the error back to userspace, which is probably
> better than spamming the logs.

Yes, let me submit a new patch for this.

Thanks,

> 
> 								Honza