linux-kernel - Re: [PATCH] exfat: retain 'VolumeFlags' properly

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ad0beeab-48ba-ee6d-f4cf-de19ec35a405@gmail.com>
Date:   Wed, 15 Jul 2020 19:06:24 +0900
From:   Tetsuhiro Kohada <kohada.t2@...il.com>
To:     Namjae Jeon <namjae.jeon@...sung.com>,
        Kohada.Tetsuhiro@...MitsubishiElectric.co.jp
Cc:     Mori.Takahiro@...MitsubishiElectric.co.jp,
        Motai.Hirotaka@...MitsubishiElectric.co.jp,
        'Sungjong Seo' <sj1557.seo@...sung.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] exfat: retain 'VolumeFlags' properly


>>>> Also, rename ERR_MEDIUM to MED_FAILURE.
>>> I think that MEDIA_FAILURE looks better.
>>
>> I think so too.
>> If so, should I change VOL_DIRTY to VOLUME_DIRTY?
> Yes, maybe.

OK.
I'll rename both in v2.


>>>> -	p_boot->vol_flags = cpu_to_le16(new_flag);
>>>> +	p_boot->vol_flags = cpu_to_le16(new_flags);
>>> How about set or clear only dirty bit to on-disk volume flags instead
>>> of using
>>> sbi->vol_flags_noclear ?
>>>         if (set)
>>>                 p_boot->vol_flags |= cpu_to_le16(VOL_DIRTY);
>>>         else
>>>                 p_boot->vol_flags &= cpu_to_le16(~VOL_DIRTY);

Please let me know about this code.
Does this code assume that 'sbi->vol_flags'(last vol_flag value) is not used?

If you use sbi->vol_flags, I think the original idea is fine.

         sbi-> vol_flags = new_flag;
	p_boot->vol_flags = cpu_to_le16(new_flag);


>> In this way, the initial  VOL_DIRTY cannot be retained.
>> The purpose of this patch is to avoid losing the initial VOL_DIRTY and MED_FAILURE.
>> Furthermore, I'm also thinking of setting MED_FAILURE.
> I know what you do. I mean this function does not need to be called if volume dirty
> Is already set on on-disk volume flags as I said earlier.

Hmm?
Does it mean the caller check that volume was dirty at mount, and caller determine
whether to call exfat_set_vol_flags() or not?
If so, MEDIA_FAILUR needs to be set independently of the volume-dirty state.


>> However, the formula for 'new_flags' may be a bit complicated.
>> Is it better to change to the following?
>>
>> 	if (new_flags == VOL_CLEAN)
>> 		new_flags = sbi->vol_flags & ~VOL_DIRTY
>> 	else
>> 		new_flags |= sbi->vol_flags;
>>
>> 	new_flags |= sbi->vol_flags_noclear;
>>
>>
>> one more point,
>> Is there a better name than 'vol_flags_noclear'?
>> (I can't come up with a good name anymore)
> It looks complicated. It would be better to simply set/clear VOLUME DIRTY bit.

I think exfat_set_vol_flags() gets a little complicated,
because it needs the followings (with bit operation)
  a) Set/Clear VOLUME_DIRTY.
  b) Set MEDIA_FAILUR.
  c) Do not change other flags.
  d) Retain VOLUME_DIRTY/MEDIA_FAILUR as it is when mounted.

'vol_flags_noclear' is used for d).

Bit-by-bit operation makes the code redundant.
I think it's not a bad way to handle multiple bits.

What do you think?


BR
---
Tetsuhiro Kohada <kohada.t2@...il.com>