linux-kernel - RE: [PATCH 2/2] mm: swap: print starting physical block offset in swapon

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: 
 <TYAPR01MB40483619A52F712D9265D4EAF6F02@TYAPR01MB4048.jpnprd01.prod.outlook.com>
Date: Mon, 27 May 2024 11:02:04 +0000
From: "Sukrit.Bhatnagar@...y.com" <Sukrit.Bhatnagar@...y.com>
To: "Darrick J. Wong" <djwong@...nel.org>
CC: "Rafael J. Wysocki" <rafael@...nel.org>, Pavel Machek <pavel@....cz>,
        Christian Brauner <brauner@...nel.org>,
        Andrew Morton
	<akpm@...ux-foundation.org>,
        "linux-xfs@...r.kernel.org"
	<linux-xfs@...r.kernel.org>,
        "linux-pm@...r.kernel.org"
	<linux-pm@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org"
	<linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: RE: [PATCH 2/2] mm: swap: print starting physical block offset in
 swapon

Hi Darrick,

On 2024-05-22 23:56, Darrick Wong wrote:
> On Wed, May 22, 2024 at 04:46:58PM +0900, Sukrit Bhatnagar wrote:
>> When a swapfile is created for hibernation purposes, we always need
>> the starting physical block offset, which is usually determined using
>> userspace commands such as filefrag.
> 
> If you always need this value, then shouldn't it be exported via sysfs
> or somewhere so that you can always get to it?  The kernel ringbuffer
> can overwrite log messages, swapfiles can get disabled, etc.

I agree on using appropriate kernel interfaces instead of kernel log.

>> It would be good to have that value printed when we do swapon and get
>> that value directly from dmesg.
>> 
>> Signed-off-by: Sukrit Bhatnagar <Sukrit.Bhatnagar@...y.com>
>> ---
>>  mm/swapfile.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index f6ca215fb92f..53c9187d5fbe 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -3264,8 +3264,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>>  		  (swap_flags & SWAP_FLAG_PRIO_MASK) >> SWAP_FLAG_PRIO_SHIFT;
>>  	enable_swap_info(p, prio, swap_map, cluster_info);
>> -	pr_info("Adding %uk swap on %s.  Priority:%d extents:%d across:%lluk %s%s%s%s\n",
>> +	pr_info("Adding %uk swap on %s. Priority:%d extents:%d start:%llu across:%lluk %s%s%s%s\n",
>>  		K(p->pages), name->name, p->prio, nr_extents,
>> +		(unsigned long long)first_se(p)->start_block,
> 
> Last time I looked, start_block was in units of PAGE_SIZE, despite
> add_swap_extent confusingly (ab)using the sector_t type.  Wherever you
> end up reporting this value, it ought to be converted to something more
> common (like byte offset or 512b-block offset).

I could not find any swap-related entries in the sysfs, but there is
/proc/swaps which shows the enabled swaps in a table.
A column for this start offset could be added there, which as you have
mentioned, should be in a unit such as bytes instead of PAGE_SIZE
blocks.

> Also ... if this is a swap *file* then reporting the path and the
> physical storage device address is not that helpful.  Exposing the block
> device major/minor and block device address would be much more useful,
> wouldn't it?

For exposing information about swap file path, I think it wouldn't make
much difference (at least for the hibernate case) as we can always do
the file-path -> bdev-path -> major:minor conversion in userspace.
 
> (Not that I have any idea what the "suspend process" in the cover letter
> refers to -- suspend and hibernate have been broken on xfs forever...)

By suspend process, I meant the series of steps taken when we trigger
hibernate's suspend-to-disk.
Not the task that started it. (Wrong choice of words, my bad).

--
Sukrit