Tookmund / Swapspace

A fork of Jeroen T. Vermeulen's excellent dynamic swap space manager
GNU General Public License v2.0
128 stars 12 forks source link

udisks failure caused by use of deprecated statfs and misuse of the glibc internal __fsword_t #18

Closed m6969p closed 4 years ago

m6969p commented 4 years ago

symptom:

swapspace calls do not contain valid file system descriptors

running swapspace creates these log entries:

`
Jan 17 12:36:25 l420 systemd[1]: Starting LSB: Linux init script for swapspace...
Jan 17 12:36:25 l420 swapspace[11248]:  * Starting dynamic swap manager swapspace
Jan 17 12:36:25 l420 swapspace[11248]: mkswap: 5: warning: wiping old swap signature.
Jan 17 12:36:25 l420 kernel: Adding 255996k swap on 5.  Priority:-3 extents:13 across:927740k FS
Jan 17 12:36:25 l420 swapspace[11248]: mkswap: 2: warning: wiping old swap signature.
Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:26 l420 kernel: Adding 255996k swap on 2.  Priority:-4 extents:5 across:1574908k FS
Jan 17 12:36:26 l420 swapspace[11248]: mkswap: 3: warning: wiping old swap signature.
Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:26 l420 kernel: Adding 255996k swap on 3.  Priority:-5 extents:1 across:255996k FS
Jan 17 12:36:26 l420 swapspace[11248]: mkswap: 4: warning: wiping old swap signature.
Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:26 l420 kernel: Adding 255996k swap on 4.  Priority:-6 extents:1 across:255996k FS
Jan 17 12:36:26 l420 swapspace[11248]: mkswap: 1: warning: wiping old swap signature.
Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:26 l420 kernel: Adding 255996k swap on 1.  Priority:-7 extents:2 across:264188k FS
Jan 17 12:36:26 l420 swapspace[11248]: mkswap: 7: warning: wiping old swap signature.
Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:27 l420 kernel: Adding 255996k swap on 7.  Priority:-8 extents:11 across:919548k FS
Jan 17 12:36:27 l420 swapspace[11248]: mkswap: 6: warning: wiping old swap signature.
Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:27 l420 kernel: Adding 255996k swap on 6.  Priority:-9 extents:2 across:264188k FS
Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:27 l420 swapspace[11248]:    ...done.
Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed
Jan 17 12:36:27 l420 systemd[1]: Started LSB: Linux init script for swapspace.
`

the culprit is the following code piece where two cardinal sins are perpetrated:

1) in a system critical component, implicating user data integrity, a deprecated function is used to determine file system and file data and properties for a critical system component, a swap file, using: statfs() (not even statfs64....) 2) and absolutely illegal, outrageously wrong in every sense: a glibc internal variable is used to determine and 'set' a property: __fsword_t the content of __fsword_t does not belong to any other but glibc and may not be altered. It should never be trusted to contain what the (at this point graduated to clueless) programmer might expect in it's shrewed view of the world where context is not a word for open source programmers.... sic ... won't even mention 'scope'... )

... and a typo, the error free case is not happening here so the fs type is actually never set correctly in the first place, which we see every 5 minutes when the swapspace cron runs....

swaps.c, from current master, pulled at the date/time of the file name: Swapspace-master-20200117115253.zip

// Store filesystem type so we don't check everytime we allocate a swapfile
__fsword_t fstype = 0;

/// Handle filesystem-specific operations needed for swapfiles
/* Some filesystems, like BTRFS, require special operations to be performed on
 * files before they can be used as swapfiles
 * Clobbers localbuf if special operations are performed
 */
void specialfs(const char path[])
{
  int err;
  // All swapfiles exist on the same filesystem, so we should only need to
  // call this once
  if (fstype == 0)
  {
    struct statfs buf;
    err = statfs(path, &buf);
        if (err != 0) fstype = buf.f_type;
  }
  if (err < 0) log_perr_str(LOG_WARNING, "Could not detect filesystem", path, errno);
  else if (fstype == BTRFS_SUPER_MAGIC)
  {
    err = runcommandformat("%s property set '%s' compression none", "btrfs", path);
    if (err != 0) logm(LOG_WARNING, "Could not disable BTRFS compression! Return Code: %d", err);
  }
}

Before anyone trusts their system, data, games, money to anything containing this 'accomplishment' I would shout out big warning: do not use swapspace before fixed and tested do not even install it and let it sit 'unused': it will clobber a system wide glibc variable!

swapspace has never been correctly tested ever, because the fs value can never be correct and it is fudging with a different subsystem's internal variable by assigning it will, does, has been causing 'weird' things wherever this was installed (just change the glibc variable to something and wait)

Why did I try swapspace in the first place: linux swapping is no good, we all know that, and browsers illegally stealing swap to hide their bad memory management and waste (not freeing what was allocated, ever...): traditional swap brings my systems to a halt because of chrome, firefox generating astronomical virtual image sizes.

Your excuse of inaction and 'nothing is wrong hgere' will be 'I did not submit this in the correct form'

This is extremely important, a red item, run with it. If you feel you need to admonish the form of this submission: so fix it and carry it further, or stay out of it.

Reproductivity: as long as there is no swap file allocated nothing happens either the moment more than 2 swap files are allocated the messages do start (or maybe once glibc has had to use it's property... defaced by swapspace....) once a couple of swap files are allocated, or when I try to remove some, the messages do appear, also every 5 minutes when swapspace runs its cron.

This needs to be tested and verified in real use on a real (test loaded) system, which very obviously has never been done. Any component that causes red lines in any log may never be released. Well, you 'can' though, but are you certain that this attitude will further the progress of open source? Smells like some ego whiff hanging in there (someone's "I don't need to read man pages", and "I don't need to heed advices like 'deprecated' and 'internal not for public use' "....)

piece of man page for ya:

_**CONFORMING TO
       Linux-specific.  The Linux statfs() was inspired by the 4.4BSD one (but they do not use the same structure).

NOTES
       The __fsword_t type used for various fields in the statfs structure definition is a glibc internal type,  not  intended
       for  public  use.   This  leaves  the programmer in a bit of a conundrum when trying to copy or compare these fields to
       local variables in a program.  Using unsigned int for such variables suffices on most systems.

       The original Linux statfs() and fstatfs() system calls were not designed with extremely large file sizes in mind.  Sub‐
       sequently,  Linux 2.6 added new statfs64() and fstatfs64() system calls that employ a new structure, statfs64.  The new
       structure contains the same fields as the original statfs structure, but the sizes of various fields are increased,  to
       accommodate  large  file  sizes.  The glibc statfs() and fstatfs() wrapper functions transparently deal with the kernel
       differences.

       Some systems have only <sys/vfs.h>, other systems also have <sys/statfs.h>, where the former includes the  latter.   So
       it seems including the former is the best choice.

       LSB has deprecated the library calls statfs() and fstatfs() and tells us to use statvfs(2) and fstatvfs(2) instead.

 Manual page statfs(2) line 178 (press h for help or q to quit)**_
Tookmund commented 4 years ago

Thank you for reporting this in such detail! I’m going to revert this entire release and remove the broken code.

Please don’t assume bad faith by default. I’ve yet to make any excuses and any inaction is only caused by lack of time due to the volunteer nature of open source.

If you have any pointers to non-deprecated functions that can give me the file system type, I’m all ears.

Jacob

On Jan 17, 2020, at 17:19, Mike notifications@github.com wrote:  symptom:

swapspace calls do not contain valid file system descriptors

running swapspace creates these log entries:

Jan 17 12:36:25 l420 systemd[1]: Starting LSB: Linux init script for swapspace... Jan 17 12:36:25 l420 swapspace[11248]: * Starting dynamic swap manager swapspace Jan 17 12:36:25 l420 swapspace[11248]: mkswap: 5: warning: wiping old swap signature. Jan 17 12:36:25 l420 kernel: Adding 255996k swap on 5. Priority:-3 extents:13 across:927740k FS Jan 17 12:36:25 l420 swapspace[11248]: mkswap: 2: warning: wiping old swap signature. Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:26 l420 kernel: Adding 255996k swap on 2. Priority:-4 extents:5 across:1574908k FS Jan 17 12:36:26 l420 swapspace[11248]: mkswap: 3: warning: wiping old swap signature. Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:26 l420 kernel: Adding 255996k swap on 3. Priority:-5 extents:1 across:255996k FS Jan 17 12:36:26 l420 swapspace[11248]: mkswap: 4: warning: wiping old swap signature. Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:26 l420 kernel: Adding 255996k swap on 4. Priority:-6 extents:1 across:255996k FS Jan 17 12:36:26 l420 swapspace[11248]: mkswap: 1: warning: wiping old swap signature. Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:26 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:26 l420 kernel: Adding 255996k swap on 1. Priority:-7 extents:2 across:264188k FS Jan 17 12:36:26 l420 swapspace[11248]: mkswap: 7: warning: wiping old swap signature. Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:27 l420 kernel: Adding 255996k swap on 7. Priority:-8 extents:11 across:919548k FS Jan 17 12:36:27 l420 swapspace[11248]: mkswap: 6: warning: wiping old swap signature. Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:27 l420 kernel: Adding 255996k swap on 6. Priority:-9 extents:2 across:264188k FS Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:27 l420 swapspace[11248]: ...done. Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:27 l420 udisksd[942]: udisks_mount_get_mount_path: assertion 'mount->type == UDISKS_MOUNT_TYPE_FILESYSTEM' failed Jan 17 12:36:27 l420 systemd[1]: Started LSB: Linux init script for swapspace. the culprit is the following code piece where two cardinal sins are perpetrated:

in a system critical component, implicating user data integrity, a deprecated function is used to determine file system and file data and properties for a critical system component, a swap file, using: statfs() (not even statfs64....) and absolutely illegal, outrageously wrong in every sense: a glibc internal variable is used to determine and 'set' a property: __fsword_t the content of __fsword_t does not belong to any other but glibc and may not be altered. It should never be trusted to contain what the (at this point graduated to clueless) programmer might expect in it's shrewed view of the world where context is not a word for open source programmers.... sic ... won't even mention 'scope'... ) ... and a typo, the error free case is not happening here so the fs type is actually never set correctly in the first place, which we see every 5 minutes when the swapspace cron runs....

swaps.c, from current master, pulled at the date/time of the file name: Swapspace-master-20200117115253.zip

// Store filesystem type so we don't check everytime we allocate a swapfile __fsword_t fstype = 0;

/// Handle filesystem-specific operations needed for swapfiles /* Some filesystems, like BTRFS, require special operations to be performed on

  • files before they can be used as swapfiles
  • Clobbers localbuf if special operations are performed */ void specialfs(const char path[]) { int err; // All swapfiles exist on the same filesystem, so we should only need to // call this once if (fstype == 0) { struct statfs buf; err = statfs(path, &buf); if (err != 0) fstype = buf.f_type; } if (err < 0) log_perr_str(LOG_WARNING, "Could not detect filesystem", path, errno); else if (fstype == BTRFS_SUPER_MAGIC) { err = runcommandformat("%s property set '%s' compression none", "btrfs", path); if (err != 0) logm(LOG_WARNING, "Could not disable BTRFS compression! Return Code: %d", err); } }

Before anyone trusts their system, data, games, money to anything containing this 'accomplishment' I would shout out big warning: do not use swapspace before fixed and tested do not even install it and let it sit 'unused': it will clobber a system wide glibc variable!

swapspace has never been correctly tested ever, because the fs value can never be correct and it is fudging with a different subsystem's internal variable by assigning it will, does, has been causing 'weird' things wherever this was installed (just change the glibc variable to something and wait)

Why did I try swapspace in the first place: linux swapping is no good, we all know that, and browsers illegally stealing swap to hide their bad memory management and waste (not freeing what was allocated, ever...): traditional swap brings my systems to a halt because of chrome, firefox generating astronomical virtual image sizes.

Your excuse of inaction and 'nothing is wrong hgere' will be 'I did not submit this in the correct form'

This is extremely important, a red item, run with it. If you feel you need to admonish the form of this submission: so fix it and carry it further, or stay out of it.

Reproductivity: as long as there is no swap file allocated nothing happens either the moment more than 2 swap files are allocated the messages do start (or maybe once glibc has had to use it's property... defaced by swapspace....) once a couple of swap files are allocated, or when I try to remove some, the messages do appear, also every 5 minutes when swapspace runs its cron.

This needs to be tested and verified in real use on a real (test loaded) system, which very obviously has never been done. Any component that causes red lines in any log may never be released. Well, you 'can' though, but are you certain that this attitude will further the progress of open source? Smells like some ego whiff hanging in there (someone's "I don't need to read man pages", and "I don't need to heed advices like 'deprecated' and 'internal not for public use' "....)

piece of man page for ya:

_**CONFORMING TO Linux-specific. The Linux statfs() was inspired by the 4.4BSD one (but they do not use the same structure).

NOTES The __fsword_t type used for various fields in the statfs structure definition is a glibc internal type, not intended for public use. This leaves the programmer in a bit of a conundrum when trying to copy or compare these fields to local variables in a program. Using unsigned int for such variables suffices on most systems.

   The original Linux statfs() and fstatfs() system calls were not designed with extremely large file sizes in mind.  Sub‐
   sequently,  Linux 2.6 added new statfs64() and fstatfs64() system calls that employ a new structure, statfs64.  The new
   structure contains the same fields as the original statfs structure, but the sizes of various fields are increased,  to
   accommodate  large  file  sizes.  The glibc statfs() and fstatfs() wrapper functions transparently deal with the kernel
   differences.

   Some systems have only <sys/vfs.h>, other systems also have <sys/statfs.h>, where the former includes the  latter.   So
   it seems including the former is the best choice.

   LSB has deprecated the library calls statfs() and fstatfs() and tells us to use statvfs(2) and fstatvfs(2) instead.

Manual page statfs(2) line 178 (press h for help or q to quit)**_

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

Tookmund commented 4 years ago

The most egregious problems have now been fixed. That still leaves the problem of statfs being deprecated, however.

Tookmund commented 4 years ago

Note that the statfs library function is transparently upgraded to statfs64 by at least glibc and musl so there's no need to explicitly call statfs64.

see glibc and musl