Bug 13622 - fruit:time machine max size is broken on arm
Summary: fruit:time machine max size is broken on arm
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 4.9.0
Hardware: Other Linux
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-09-18 13:05 UTC by Pavel Volkovitskiy
Modified: 2020-03-19 09:29 UTC (History)
3 users (show)

See Also:


Attachments
Fixes tm_size overflow check code on armv7l platform (1016 bytes, patch)
2018-12-10 08:48 UTC, Pavel Volkovitskiy
no flags Details
git-am fix for 4.12.next, 4.11.next (1.93 KB, patch)
2020-03-08 18:19 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Volkovitskiy 2018-09-18 13:05:50 UTC
I'm starting samba with
fruit:time machine max size = 700G

and get error:
fruit_tmsize_do_dirent: tmsize overflow: bandsize [8388608] nbands [82339]

bandsize and nbands values looks right and this config was working with previous samba version

I've found this commit:
vfs_fruit: Fix CID 1433613 Operands don't affect result
https://git.samba.org/samba.git/?p=samba.git;a=commit;h=f3e98f41de25e3b52fd38f86317e428bfb53b287

6558         if (bandsize > SIZE_MAX/nbands) {
6559                 DBG_ERR("tmsize overflow: bandsize [%zu] nbands [%zu]\n",
6560                         bandsize, nbands);
6561                 return false;

and it seems SIZE_MAX is only 4GB-1: 4294967295

I'm building samba on debian buster in docker container on armv7l hardware
Comment 1 Volker Lendecke 2018-09-18 13:41:36 UTC
Hi, Ralph!

It seems that much of the time machine stuff uses size_t, which on this platform looks like just 32-bit. A fix would be to convert all of that to uint64_t. Correct?
Comment 2 Ralph Böhme 2018-09-18 23:07:25 UTC
Or maybe off_t which is what struct stat uses for the file size member.
Comment 3 Pavel Volkovitskiy 2018-12-10 08:46:40 UTC
This problem still exists, with samba 4.9.3

I've got an error:
fruit_tmsize_do_dirent: tmsize overflow: bandsize [8388608] nbands [102927]

from this code:
if (bandsize > SIZE_MAX/nbands) {
    DBG_ERR("tmsize overflow: bandsize [%zu] nbands [%zu]\n",
            bandsize, nbands);
    return false;
}
tm_size = bandsize * nbands;

This "if" tries to protect tm_size from overflow.
And the type of tm_size is off_t already.

So it's just wrong assumption here, that SIZE_MAX == OFF_T_MAX

Unfortunately there is no cross-platform OFF_T_MAX macros, so I've declared it this way:

#ifndef OFF_T_MAX
#if SIZEOF_OFF_T == SIZEOF_INT8_T
#define OFF_T_MAX INT8_MAX
#elif SIZEOF_OFF_T == SIZEOF_INT16_T
#define OFF_T_MAX INT16_MAX
#elif SIZEOF_OFF_T == SIZEOF_INT32_T
#define OFF_T_MAX INT32_MAX
#elif SIZEOF_OFF_T == SIZEOF_INT64_T
#define OFF_T_MAX INT64_MAX
#endif
#endif

and replaced SIZE_MAX with OFF_T_MAX in that "if"

this fixed issue for me
Comment 4 Pavel Volkovitskiy 2018-12-10 08:48:30 UTC
Created attachment 14726 [details]
Fixes tm_size overflow check code on armv7l platform
Comment 5 Oleg Semyonov 2019-05-03 11:15:57 UTC
With current file sizes having tm_size of 32 bits is useless, so it should be at least 64 bits. As such, the following code seems to be unnecessary:

	if (bandsize > SIZE_MAX/nbands) {
		DBG_ERR("tmsize overflow: bandsize [%zu] nbands [%zu]\n",
			bandsize, nbands);
		return false;
	}

This, perhaps, too:

	if (state->total_size + tm_size < state->total_size) {
		DBG_ERR("tmsize overflow: bandsize [%zu] nbands [%zu]\n",
			bandsize, nbands);
		return false;
	}

And after removing the first (or both) only the following line should be fixed:

	tm_size = bandsize * nbands;

replaced by:

	tm_size = (off_t)bandsize * (off_t)nbands;

Otherwise tm_size is calculated in 32 bits and loses higher 32 bits (looks like only up to 4G is used instead of 100-200+).

Other option is to do all calculations in uint64_t instead of off_t, but it depends on personal taste (use off_t for file sizes or uint64_t).
Comment 6 Oleg Semyonov 2019-05-03 13:44:39 UTC
Removed both 'if' clauses and added (off_t) as shown above.
Built package for Debian, armel architecture, sizeof(off_t)=8, sizeof(size_t)=4.

Confirmed, free and used space reported correctly for TM shares.
Comment 7 Louis 2019-05-06 08:54:17 UTC
Hai, 

I've gotten a report also thats this is seen on Debian 9, 64bit. samba 4.9.7 
I've ask the list reporter to update the bugreport here also. 

his report. 

MacOS can’t seem to get the size of the disk.

I’ve traced it down to this config option
fruit:time machine max size = 326G

Which results in

[Sun May  5 15:47:28 2019] traps: smbd[9658] trap divide error ip:7f8d722a84bb sp:7ffdf4e93cb0 error:0
[Sun May  5 15:47:28 2019]  in fruit.so[7f8d7229f000+1c000]

If I remove the max size option, all is well
Comment 8 Oleg Semyonov 2019-05-06 09:04:31 UTC
From the code:

	if (bandsize > SIZE_MAX/nbands) {
		DBG_ERR("tmsize overflow: bandsize [%zu] nbands [%zu]\n",
			bandsize, nbands);
		return false;
	}

it follows that when you have no TM bands yet (before the 1st backup), it will zero divide. And, as I said, this 'if' is useless when you operate with 64 bit values (it always will be less than uint64_t max value).

Removing both 'if' and adding type conversion should fix all reported issues and make max size option work. At least, it worked for me for both new and incremental backups.
Comment 9 andieq 2020-02-09 16:41:52 UTC
This still seem to-be an issue in 4.11.6, is there a reason this has not been patched? 
The last comment on this and fix seem rather trivial.
Comment 10 Jeremy Allison 2020-03-08 18:19:51 UTC
Created attachment 15850 [details]
git-am fix for 4.12.next, 4.11.next

Cherry-picked from master.
Comment 11 Ralph Böhme 2020-03-16 13:16:03 UTC
Reassigning to Karolin for inclusion in 4.11 and 4.12.
Comment 12 Karolin Seeger 2020-03-18 10:21:17 UTC
(In reply to Ralph Böhme from comment #11)
Pushed to autobuild-v4-{12,11}-test.
Comment 13 Karolin Seeger 2020-03-19 09:29:33 UTC
(In reply to Karolin Seeger from comment #12)
Pushed to both branches.
Closing out bug report.

Thanks!