linux-kernel - Re: [PATCH] drivers/cdrom: improved ioctl for media change detection

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: <5f3b7d8b-9e97-094b-efd1-cad6cab793b6@kernel.dk>
Date:   Fri, 27 Aug 2021 21:18:16 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Lukas Prediger <lumip@...ip.de>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers/cdrom: improved ioctl for media change detection

On 8/27/21 11:30 AM, Lukas Prediger wrote:
>>> @@ -295,6 +297,19 @@ struct cdrom_generic_command
>>>  	};
>>>  };
>>>  
>>> +/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
>>> +struct cdrom_timed_media_change_info
>>> +{
>>> +	__u64   last_media_change;	/* Timestamp of the last detected media
>>> +					 * change. May be set by caller, updated
>>> +					 * upon successful return of ioctl.
>>> +					 */
>>> +	__u8    has_changed;		/* Set to 1 by ioctl if last detected media
>>> +					 * change was more recent than
>>> +					 * last_media_change set by caller.
>>> +					 */
>>> +};
>>>
>> The struct layout should be modified such that there are no holes or
>> padding in it. Probably just make the has_changed a flags thing, and
>> make it u64 as well. Then you can define bit 0 to be HAS_CHANGED, and
>> that leaves you room to add more flags in the future. Though the latter
>> probably isn't much of a concern here, but...
> 
> 1. jiffies_to_msecs returns unsigned int. Should I reflect that in the
> struct (i.e., make the last_media_change and has_changed fields also
> of type unsigned int or should I keep them at a fixed bit width?

You can make it an u32. Always use explicitly sized types for user API
structures.

> 2. As the last_media_change field will be in ms now, is it safe to
> convert those back to jiffies for comparison or is there a risk of
> information loss (due to rounding or whatever) in either conversion?
> More technically, can I make the assumption that for any jiffies value
> x it holds that

The granularity of jiffies depends on the HZ setting, generally just
consider it somewhere in between 100 and 1000. That's where my initial
granularity numbers came from.

> time_before(msecs_to_jiffies(jiffies_to_msecs(x)), x) is always false ?

I don't think that matters. Internally, always keep things in jiffies.
That's what you use to compare with, and to check if it's changed since
last time. The only time you convert to ms is to pass it back to
userspace. And that's going to be a delta of jiffies always, just ensure
you assign last_checked = jiffies when it's setup initially.

-- 
Jens Axboe