| Message ID | 20210316085757.3807410-1-dominique.martinet@atmark-techno.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC] core/stream_interface: add free space checks before writing file copies | expand |
Hi Dominique, On 16.03.21 09:57, Dominique Martinet wrote: > copyfile would normally fail with ENOSPC on write, but if we can know > beforehand that the file will not fit it is better to error early and > not distrupt whatever is running. Note that copyfile cannot check in advanmce because the output file descriptor could be a pipe. > > Note that the check is not perfect: if the filesystem has transparent > compression and the data is compressible then the file could maybe > be written despite not having enough free blocks to fit, but we have > no way of knowing so fail anyway. There is no way to know in advance, we should pass in sw-description the size of the uncompressed data just for this check, but it is clumsy. > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > > I'm a bit torn about this one wrt. portability: statvfs is in posix so > should be mostly available but the freebsd man page recommends not > using it. > It'd probably be better to have two different linux and bsd fstatfs() > implementations, but I don't have any bsd around to try. Adde Christian in CC, he added support for BSD. If Christian complains, you can surround your code with #if defined(__FreeBSD__) > > The compression problem I gave in commit message is also probably worth > considering and adding a no-check switch to swupdate commandline? > > > Anyway, this appears to work on linux within the limitations it has. > Ok > > core/stream_interface.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/core/stream_interface.c b/core/stream_interface.c > index d459010886b5..61b3e91c19d8 100644 > --- a/core/stream_interface.c > +++ b/core/stream_interface.c > @@ -25,6 +25,7 @@ > #include <sys/mman.h> > #include <sys/reboot.h> > #include <sys/stat.h> > +#include <sys/statvfs.h> > #include <pthread.h> > #include "cpiohdr.h" > > @@ -73,6 +74,23 @@ pthread_cond_t stream_wkup = PTHREAD_COND_INITIALIZER; > > static struct installer inst; > > +static int check_free_space(int fdout, struct filehdr *fdh, char *filepath) > +{ > + struct statvfs statvfs; > + > + if (fstatvfs(fdout, &statvfs)) { > + ERROR("statvfs failed on %s, skipping free space check", > + filepath); > + } else { > + if (statvfs.f_bfree * statvfs.f_bsize < fdh->size) { > + ERROR("Not enough free space to store temporary copy of %s (needed %zu, got %lu), aborting", > + fdh->filename, fdh->size, > + statvfs.f_bfree * statvfs.f_bsize); > + return -1; I have already done the error to use int as boolean, I am cleaning up and replacing int with boolean when no useful return code is sent back as in this case. So change signature of function as "static bool" > + } > + } > + return 0; > +} > static int extract_file_to_tmp(int fd, const char *fname, unsigned long *poffs, bool encrypted) > { > char output_file[MAX_IMAGE_FNAME]; > @@ -103,6 +121,9 @@ static int extract_file_to_tmp(int fd, const char *fname, unsigned long *poffs, > if (fdout < 0) > return -1; > > + if (check_free_space(fdout, &fdh, output_file) < 0) > + return -1; > + > if (copyfile(fd, &fdout, fdh.size, poffs, 0, 0, 0, &checksum, NULL, > encrypted, NULL, NULL) < 0) { > close(fdout); > @@ -222,6 +243,8 @@ static int extract_files(int fd, struct swupdate_cfg *software) > fdout = openfileoutput(img->extract_file); > if (fdout < 0) > return -1; > + if (check_free_space(fdout, &fdh, img->extract_file) < 0) > + return -1; > if (copyfile(fd, &fdout, fdh.size, &offset, 0, 0, 0, &checksum, img->sha256, false, NULL, NULL) < 0) { > close(fdout); > return -1; > Best regards, Stefano Babic
Hi, > > I'm a bit torn about this one wrt. portability: statvfs is in posix so > > should be mostly available but the freebsd man page recommends not > > using it. > > It'd probably be better to have two different linux and bsd fstatfs() > > implementations, but I don't have any bsd around to try. > > Adde Christian in CC, he added support for BSD. If Christian complains, you > can surround your code with #if defined(__FreeBSD__) Quoting the manpage: "fstatvfs() functions fill the structure pointed to by buf with garbage. This garbage will occasionally bear resemblance to file system statistics, but portable applications must not depend on this." After all, they do a have certain sense of humor :) Further, it says: "The statvfs() and fstatvfs() functions are implemented as wrappers around the statfs() and fstatfs() functions, respectively." I do not have a FreeBSD system in reach right now in home office times, but the manpage suggests to use fstatfs() directly. So, I cannot tell right now but *assume* the "garbage" it puts into buf makes some sense... I have to test a recent SWUpdate anyway on FreeBSD 13 so I can also test this then... Kind regards, Christian
Stefano Babic wrote on Tue, Mar 16, 2021 at 12:35:03PM +0100: > On 16.03.21 09:57, Dominique Martinet wrote: > > copyfile would normally fail with ENOSPC on write, but if we can know > > beforehand that the file will not fit it is better to error early and > > not distrupt whatever is running. > > Note that copyfile cannot check in advanmce because the output file > descriptor could be a pipe. Yes, that's why I added the check to extract_files and extract_file_to_tmp, it's better than nothing. > > Note that the check is not perfect: if the filesystem has transparent > > compression and the data is compressible then the file could maybe > > be written despite not having enough free blocks to fit, but we have > > no way of knowing so fail anyway. > > There is no way to know in advance, we should pass in sw-description the > size of the uncompressed data just for this check, but it is clumsy. Oh, I didn't even think of data we compress ourselves.. But that is correct we only know the size in the cpio archive (likely compressed). I was referring here to free space reported by the filesystem vs. size we could actually write through filesystem compression (e.g. zfs/btrfs mounted with compression enabled), which we also have no way of knowing. Well, we already have checksum that needs to be filled in sw-description everytime the files change, it wouldn't be much worse to add an optional free space check size attribute and only run statvfs if that is set and ignore anything else? > > I'm a bit torn about this one wrt. portability: statvfs is in posix so > > should be mostly available but the freebsd man page recommends not > > using it. > > It'd probably be better to have two different linux and bsd fstatfs() > > implementations, but I don't have any bsd around to try. > > Adde Christian in CC, he added support for BSD. If Christian complains, you > can surround your code with #if defined(__FreeBSD__) Thanks, please tell me what you prefer. > > + if (statvfs.f_bfree * statvfs.f_bsize < fdh->size) { > > + ERROR("Not enough free space to store temporary copy of %s (needed %zu, got %lu), aborting", > > + fdh->filename, fdh->size, > > + statvfs.f_bfree * statvfs.f_bsize); > > + return -1; > > I have already done the error to use int as boolean, I am cleaning up and > replacing int with boolean when no useful return code is sent back as in > this case. So change signature of function as "static bool" Ok, will do once we have decided what to base the check on.
(Sorry, had missed the reply as I had been dropped from Ccs, will adjust my filters a bit) Christian Storm wrote on Tue, Mar 16, 2021 at 05:43:49PM +0100: > Further, it says: > "The statvfs() and fstatvfs() functions are implemented as wrappers around > the statfs() and fstatfs() functions, respectively." > > I do not have a FreeBSD system in reach right now in home office times, > but the manpage suggests to use fstatfs() directly. So, I cannot tell > right now but *assume* the "garbage" it puts into buf makes some > sense... I have to test a recent SWUpdate anyway on FreeBSD 13 so I can > also test this then... That is my understanding as well, the statvfs struct should contain mostly sensible informations based on what statfs returns. My main worry is the f_bsize field: f_bsize The preferred length of I/O requests for files on this file system. (Corresponds to the f_iosize member of struct statfs.) where the check would want to use statfs' f_bsize instead: uint64_t f_bsize; /* filesystem fragment size */ so it would probably be best to implment linux/bsd with their own statfs() each and pick the appropiate fields with ifdefs If you're willing to test I can prepare a patch with that in mind. Thanks,
Hi Dominique, On 17.03.21 01:26, Dominique Martinet wrote: > Stefano Babic wrote on Tue, Mar 16, 2021 at 12:35:03PM +0100: >> On 16.03.21 09:57, Dominique Martinet wrote: >>> copyfile would normally fail with ENOSPC on write, but if we can know >>> beforehand that the file will not fit it is better to error early and >>> not distrupt whatever is running. >> >> Note that copyfile cannot check in advanmce because the output file >> descriptor could be a pipe. > > Yes, that's why I added the check to extract_files and > extract_file_to_tmp, it's better than nothing. > >>> Note that the check is not perfect: if the filesystem has transparent >>> compression and the data is compressible then the file could maybe >>> be written despite not having enough free blocks to fit, but we have >>> no way of knowing so fail anyway. >> >> There is no way to know in advance, we should pass in sw-description the >> size of the uncompressed data just for this check, but it is clumsy. > > Oh, I didn't even think of data we compress ourselves.. But that is > correct we only know the size in the cpio archive (likely compressed). > > I was referring here to free space reported by the filesystem vs. size > we could actually write through filesystem compression (e.g. zfs/btrfs > mounted with compression enabled), which we also have no way of knowing. > > > Well, we already have checksum that needs to be filled in sw-description > everytime the files change, Thins is that this is required to verify the integrity of the file, while the size is fully optional. SWUpdate will stop with an error if there is no enough space, and this information is mostly redundant. We can also have a mismatch between "uncompressed-size" in sw-description and and the real size, and then what should be done ? There is currently only a "decompressed-size" for the UBIU handler as property, because UBI APIU in kernel requires to set the size before streaming the data. > it wouldn't be much worse to add an optional > free space check size attribute and only run statvfs if that is set and > ignore anything else? It could be possible, see my remarks above. This can overlaps with the attribute in UBI handler that cannot be changed due to compatibility with the past. > >>> I'm a bit torn about this one wrt. portability: statvfs is in posix so >>> should be mostly available but the freebsd man page recommends not >>> using it. >>> It'd probably be better to have two different linux and bsd fstatfs() >>> implementations, but I don't have any bsd around to try. >> >> Adde Christian in CC, he added support for BSD. If Christian complains, you >> can surround your code with #if defined(__FreeBSD__) > > Thanks, please tell me what you prefer. > >>> + if (statvfs.f_bfree * statvfs.f_bsize < fdh->size) { >>> + ERROR("Not enough free space to store temporary copy of %s (needed %zu, got %lu), aborting", >>> + fdh->filename, fdh->size, >>> + statvfs.f_bfree * statvfs.f_bsize); >>> + return -1; >> >> I have already done the error to use int as boolean, I am cleaning up and >> replacing int with boolean when no useful return code is sent back as in >> this case. So change signature of function as "static bool" > > Ok, will do once we have decided what to base the check on. > Regards, Stefano
On 17.03.21 02:03, Dominique MARTINET wrote: > > (Sorry, had missed the reply as I had been dropped from Ccs, will adjust > my filters a bit) > > Christian Storm wrote on Tue, Mar 16, 2021 at 05:43:49PM +0100: >> Further, it says: >> "The statvfs() and fstatvfs() functions are implemented as wrappers around >> the statfs() and fstatfs() functions, respectively." >> >> I do not have a FreeBSD system in reach right now in home office times, >> but the manpage suggests to use fstatfs() directly. So, I cannot tell >> right now but *assume* the "garbage" it puts into buf makes some >> sense... I have to test a recent SWUpdate anyway on FreeBSD 13 so I can >> also test this then... > > That is my understanding as well, the statvfs struct should contain > mostly sensible informations based on what statfs returns. > > My main worry is the f_bsize field: > f_bsize The preferred length of I/O requests for files on this > file system. (Corresponds to the f_iosize member of > struct statfs.) > > where the check would want to use statfs' f_bsize instead: > uint64_t f_bsize; /* filesystem fragment size */ > > so it would probably be best to implment linux/bsd with their own > statfs() each and pick the appropiate fields with ifdefs Right, just write a wrapper that is independent from OS and put the #ifdef in util.h. > If you're willing to test I can prepare a patch with that in mind. > Regards, Stefano
Stefano Babic wrote on Wed, Mar 17, 2021 at 10:33:35AM +0100: > > Well, we already have checksum that needs to be filled in sw-description > > everytime the files change, > > Thins is that this is required to verify the integrity of the file, while > the size is fully optional. SWUpdate will stop with an error if there is no > enough space, and this information is mostly redundant. We can also have a > mismatch between "uncompressed-size" in sw-description and and the real > size, and then what should be done ? I guess that depends on what you use that size for. If it's purely a free space check, it doesn't matter if it's not accurate -- it can be used once before extracting anywhere and never again, there is little point in comparing with the actual size (input validation is the checksum's job, not this attribute's) > There is currently only a "decompressed-size" for the UBIU handler as > property, because UBI APIU in kernel requires to set the size before > streaming the data. .. unless the handler wants to do more with it, of course. In this case I expect the kernel to report an error if the size was incorrect? > > it wouldn't be much worse to add an optional > > free space check size attribute and only run statvfs if that is set and > > ignore anything else? > > It could be possible, see my remarks above. This can overlaps with the > attribute in UBI handler that cannot be changed due to compatibility with > the past. I guess we can use the same name. I'll send a v2 with that and the two statfs() implementation, probably on Friday.
On 17.03.21 12:07, Dominique Martinet wrote: > Stefano Babic wrote on Wed, Mar 17, 2021 at 10:33:35AM +0100: >>> Well, we already have checksum that needs to be filled in sw-description >>> everytime the files change, >> >> Thins is that this is required to verify the integrity of the file, while >> the size is fully optional. SWUpdate will stop with an error if there is no >> enough space, and this information is mostly redundant. We can also have a >> mismatch between "uncompressed-size" in sw-description and and the real >> size, and then what should be done ? > > I guess that depends on what you use that size for. > If it's purely a free space check, it doesn't matter if it's not > accurate -- it can be used once before extracting anywhere and never > again, there is little point in comparing with the actual size (input > validation is the checksum's job, not this attribute's) Right - I do not know if the name should be "size", it is more an optional precondition. If it is named "size", people expects that the artifact is exactly of that size. > >> There is currently only a "decompressed-size" for the UBIU handler as >> property, because UBI APIU in kernel requires to set the size before >> streaming the data. > > .. unless the handler wants to do more with it, of course. > In this case I expect the kernel to report an error if the size was > incorrect? Your expectation is correct. > > >>> it wouldn't be much worse to add an optional >>> free space check size attribute and only run statvfs if that is set and >>> ignore anything else? >> >> It could be possible, see my remarks above. This can overlaps with the >> attribute in UBI handler that cannot be changed due to compatibility with >> the past. > > I guess we can use the same name. > I'll send a v2 with that and the two statfs() implementation, probably > on Friday. > Ok,, thanks. Best regards, Stefano Babic
diff --git a/core/stream_interface.c b/core/stream_interface.c index d459010886b5..61b3e91c19d8 100644 --- a/core/stream_interface.c +++ b/core/stream_interface.c @@ -25,6 +25,7 @@ #include <sys/mman.h> #include <sys/reboot.h> #include <sys/stat.h> +#include <sys/statvfs.h> #include <pthread.h> #include "cpiohdr.h" @@ -73,6 +74,23 @@ pthread_cond_t stream_wkup = PTHREAD_COND_INITIALIZER; static struct installer inst; +static int check_free_space(int fdout, struct filehdr *fdh, char *filepath) +{ + struct statvfs statvfs; + + if (fstatvfs(fdout, &statvfs)) { + ERROR("statvfs failed on %s, skipping free space check", + filepath); + } else { + if (statvfs.f_bfree * statvfs.f_bsize < fdh->size) { + ERROR("Not enough free space to store temporary copy of %s (needed %zu, got %lu), aborting", + fdh->filename, fdh->size, + statvfs.f_bfree * statvfs.f_bsize); + return -1; + } + } + return 0; +} static int extract_file_to_tmp(int fd, const char *fname, unsigned long *poffs, bool encrypted) { char output_file[MAX_IMAGE_FNAME]; @@ -103,6 +121,9 @@ static int extract_file_to_tmp(int fd, const char *fname, unsigned long *poffs, if (fdout < 0) return -1; + if (check_free_space(fdout, &fdh, output_file) < 0) + return -1; + if (copyfile(fd, &fdout, fdh.size, poffs, 0, 0, 0, &checksum, NULL, encrypted, NULL, NULL) < 0) { close(fdout); @@ -222,6 +243,8 @@ static int extract_files(int fd, struct swupdate_cfg *software) fdout = openfileoutput(img->extract_file); if (fdout < 0) return -1; + if (check_free_space(fdout, &fdh, img->extract_file) < 0) + return -1; if (copyfile(fd, &fdout, fdh.size, &offset, 0, 0, 0, &checksum, img->sha256, false, NULL, NULL) < 0) { close(fdout); return -1;
copyfile would normally fail with ENOSPC on write, but if we can know beforehand that the file will not fit it is better to error early and not distrupt whatever is running. Note that the check is not perfect: if the filesystem has transparent compression and the data is compressible then the file could maybe be written despite not having enough free blocks to fit, but we have no way of knowing so fail anyway. Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- I'm a bit torn about this one wrt. portability: statvfs is in posix so should be mostly available but the freebsd man page recommends not using it. It'd probably be better to have two different linux and bsd fstatfs() implementations, but I don't have any bsd around to try. The compression problem I gave in commit message is also probably worth considering and adding a no-check switch to swupdate commandline? Anyway, this appears to work on linux within the limitations it has. core/stream_interface.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)