| Message ID | 20230904084547.2209709-1-lukas.funke-oss@weidmueller.com |
|---|---|
| State | Rejected |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [1/2] stream_interface: do not remove files recursivly by default | expand |
Hi Lukas, On 04.09.23 10:45, lukas.funke-oss@weidmueller.com wrote: > From: Lukas Funke <lukas.funke@weidmueller.com> > > Files in DATADST_DIR belong to a mount point. The device > is usually unmounted from this mountpoint and the directory > is expected to be empty. However, in case of an error the > mount may still exist. Then there could be files in the > DATADST_DIR folder. Removing the files non-recursivly should then > fail if the directory is non-empty. > No. The DATADST_DIR is thought to be a temporary directory where artifacts are extracted (if streaming is deactivated). The life of files in DATADST_DIR should not survive an update session. After SWUpdate has finished, data should be removed to avoid any possible memory between further updates. Think also that letting these files could be (is it ? ) a security leak. In fact, it could be possible to install something in DATADST_DIR during a broken update, and using them in some way later. I imagine which is your use case, but this is the wrong approach. SWUpdate mount / unmount during an upüdate to let the system in a clean state. If unmount fails, someone in between is starting to access the files causing the filesystem to be busy, but this is not allowed until the update is declared successful. This patch will be rejected. Best regards, Stefano Babic > Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com> > --- > core/stream_interface.c | 4 ++-- > core/util.c | 12 ++++++++++-- > include/util.h | 2 +- > 3 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/core/stream_interface.c b/core/stream_interface.c > index 3f0b952..e03fc38 100644 > --- a/core/stream_interface.c > +++ b/core/stream_interface.c > @@ -696,8 +696,8 @@ void *network_initializer(void *data) > cleanup_files(software); > > #ifndef CONFIG_NOCLEANUP > - swupdate_remove_directory(SCRIPTS_DIR_SUFFIX); > - swupdate_remove_directory(DATADST_DIR_SUFFIX); > + swupdate_remove_directory(SCRIPTS_DIR_SUFFIX, true); > + swupdate_remove_directory(DATADST_DIR_SUFFIX, false); > #endif > > pthread_mutex_lock(&stream_mutex); > diff --git a/core/util.c b/core/util.c > index a6ddf82..af22590 100644 > --- a/core/util.c > +++ b/core/util.c > @@ -172,7 +172,7 @@ static int _remove_directory_cb(const char *fpath, const struct stat *sb, > return remove(fpath); > } > > -int swupdate_remove_directory(const char* path) > +int swupdate_remove_directory(const char* path, bool recursive) > { > char* dpath; > int ret; > @@ -181,7 +181,15 @@ int swupdate_remove_directory(const char* path) > ERROR("OOM: Directory %s not removed", path); > return -ENOMEM; > } > - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > + > + if (recursive) { > + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); > + } else { > + ret = rmdir(dpath); > + if (ret) > + ERROR("Remove directory %s failed: %s", dpath, strerror(errno)); > + } > + > free(dpath); > return ret; > } > diff --git a/include/util.h b/include/util.h > index 3c2a42f..9c21949 100644 > --- a/include/util.h > +++ b/include/util.h > @@ -258,7 +258,7 @@ const char* get_tmpdirscripts(void); > > void swupdate_create_directory(const char* path); > #ifndef CONFIG_NOCLEANUP > -int swupdate_remove_directory(const char* path); > +int swupdate_remove_directory(const char* path, bool recursive); > #endif > > int swupdate_mount(const char *device, const char *dir, const char *fstype);
Hi Stefano, On 04.09.2023 11:15, Stefano Babic wrote: > Hi Lukas, > > On 04.09.23 10:45, lukas.funke-oss@weidmueller.com wrote: >> From: Lukas Funke <lukas.funke@weidmueller.com> >> >> Files in DATADST_DIR belong to a mount point. The device >> is usually unmounted from this mountpoint and the directory >> is expected to be empty. However, in case of an error the >> mount may still exist. Then there could be files in the >> DATADST_DIR folder. Removing the files non-recursivly should then >> fail if the directory is non-empty. >> > > No. > > The DATADST_DIR is thought to be a temporary directory where artifacts > are extracted (if streaming is deactivated). The life of files in > DATADST_DIR should not survive an update session. After SWUpdate has > finished, data should be removed to avoid any possible memory between > further updates. > > Think also that letting these files could be (is it ? ) a security leak. > In fact, it could be possible to install something in DATADST_DIR during > a broken update, and using them in some way later. > > I imagine which is your use case, but this is the wrong approach. > SWUpdate mount / unmount during an upüdate to let the system in a clean > state. If unmount fails, someone in between is starting to access the > files causing the filesystem to be busy, but this is not allowed until > the update is declared successful. > > This patch will be rejected. Thanks for the clarification. The idea behind the patch is, if someone installs a bootloader using this way and - for whatever reason - the unmount fails, then the files in DATADST_DIR should not be removed, because then the bootloader would be removed, resulting in a bricked system. > > Best regards, > Stefano Babic > >> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com> >> --- >> core/stream_interface.c | 4 ++-- >> core/util.c | 12 ++++++++++-- >> include/util.h | 2 +- >> 3 files changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/core/stream_interface.c b/core/stream_interface.c >> index 3f0b952..e03fc38 100644 >> --- a/core/stream_interface.c >> +++ b/core/stream_interface.c >> @@ -696,8 +696,8 @@ void *network_initializer(void *data) >> cleanup_files(software); >> #ifndef CONFIG_NOCLEANUP >> - swupdate_remove_directory(SCRIPTS_DIR_SUFFIX); >> - swupdate_remove_directory(DATADST_DIR_SUFFIX); >> + swupdate_remove_directory(SCRIPTS_DIR_SUFFIX, true); >> + swupdate_remove_directory(DATADST_DIR_SUFFIX, false); >> #endif >> pthread_mutex_lock(&stream_mutex); >> diff --git a/core/util.c b/core/util.c >> index a6ddf82..af22590 100644 >> --- a/core/util.c >> +++ b/core/util.c >> @@ -172,7 +172,7 @@ static int _remove_directory_cb(const char *fpath, >> const struct stat *sb, >> return remove(fpath); >> } >> -int swupdate_remove_directory(const char* path) >> +int swupdate_remove_directory(const char* path, bool recursive) >> { >> char* dpath; >> int ret; >> @@ -181,7 +181,15 @@ int swupdate_remove_directory(const char* path) >> ERROR("OOM: Directory %s not removed", path); >> return -ENOMEM; >> } >> - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); >> + >> + if (recursive) { >> + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | >> FTW_PHYS); >> + } else { >> + ret = rmdir(dpath); >> + if (ret) >> + ERROR("Remove directory %s failed: %s", dpath, >> strerror(errno)); >> + } >> + >> free(dpath); >> return ret; >> } >> diff --git a/include/util.h b/include/util.h >> index 3c2a42f..9c21949 100644 >> --- a/include/util.h >> +++ b/include/util.h >> @@ -258,7 +258,7 @@ const char* get_tmpdirscripts(void); >> void swupdate_create_directory(const char* path); >> #ifndef CONFIG_NOCLEANUP >> -int swupdate_remove_directory(const char* path); >> +int swupdate_remove_directory(const char* path, bool recursive); >> #endif >> int swupdate_mount(const char *device, const char *dir, const char >> *fstype); >
Hi Lukas, On 04.09.23 11:45, Lukas Funke wrote: > Hi Stefano, > > On 04.09.2023 11:15, Stefano Babic wrote: >> Hi Lukas, >> >> On 04.09.23 10:45, lukas.funke-oss@weidmueller.com wrote: >>> From: Lukas Funke <lukas.funke@weidmueller.com> >>> >>> Files in DATADST_DIR belong to a mount point. The device >>> is usually unmounted from this mountpoint and the directory >>> is expected to be empty. However, in case of an error the >>> mount may still exist. Then there could be files in the >>> DATADST_DIR folder. Removing the files non-recursivly should then >>> fail if the directory is non-empty. >>> >> >> No. >> >> The DATADST_DIR is thought to be a temporary directory where artifacts >> are extracted (if streaming is deactivated). The life of files in >> DATADST_DIR should not survive an update session. After SWUpdate has >> finished, data should be removed to avoid any possible memory between >> further updates. >> >> Think also that letting these files could be (is it ? ) a security >> leak. In fact, it could be possible to install something in >> DATADST_DIR during a broken update, and using them in some way later. >> >> I imagine which is your use case, but this is the wrong approach. >> SWUpdate mount / unmount during an upüdate to let the system in a >> clean state. If unmount fails, someone in between is starting to >> access the files causing the filesystem to be busy, but this is not >> allowed until the update is declared successful. >> >> This patch will be rejected. > > Thanks for the clarification. The idea behind the patch is, if someone > installs a bootloader using this way and - for whatever reason - the > unmount fails, Well, putting the bootloader on a filesystem is *always* a bad idea. There just a few cases where this is required (UEFI on EFI partition), else it is safer to store it where the vendor suggests on raw flash. Which is the reason the umount at that point fails ? It is seen as transaction: swupdate_mount(); .....install anything umount(); And SWUpdate is designed to be sync with any handler (no concurrency of handlers). > then the files in DATADST_DIR should not be removed, This happens with any artifact, but if umount() fails, it is supposed something is going wrong. When does it happen on your site ? The only improvement I see is that handlers can use a new temporary mount point instead of reusing DATADST_DIR, as the scope / goal is different. > because then the bootloader would be removed, resulting in a bricked > system. > Best regards, Stefano Babic >> >>> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com> >>> --- >>> core/stream_interface.c | 4 ++-- >>> core/util.c | 12 ++++++++++-- >>> include/util.h | 2 +- >>> 3 files changed, 13 insertions(+), 5 deletions(-) >>> >>> diff --git a/core/stream_interface.c b/core/stream_interface.c >>> index 3f0b952..e03fc38 100644 >>> --- a/core/stream_interface.c >>> +++ b/core/stream_interface.c >>> @@ -696,8 +696,8 @@ void *network_initializer(void *data) >>> cleanup_files(software); >>> #ifndef CONFIG_NOCLEANUP >>> - swupdate_remove_directory(SCRIPTS_DIR_SUFFIX); >>> - swupdate_remove_directory(DATADST_DIR_SUFFIX); >>> + swupdate_remove_directory(SCRIPTS_DIR_SUFFIX, true); >>> + swupdate_remove_directory(DATADST_DIR_SUFFIX, false); >>> #endif >>> pthread_mutex_lock(&stream_mutex); >>> diff --git a/core/util.c b/core/util.c >>> index a6ddf82..af22590 100644 >>> --- a/core/util.c >>> +++ b/core/util.c >>> @@ -172,7 +172,7 @@ static int _remove_directory_cb(const char >>> *fpath, const struct stat *sb, >>> return remove(fpath); >>> } >>> -int swupdate_remove_directory(const char* path) >>> +int swupdate_remove_directory(const char* path, bool recursive) >>> { >>> char* dpath; >>> int ret; >>> @@ -181,7 +181,15 @@ int swupdate_remove_directory(const char* path) >>> ERROR("OOM: Directory %s not removed", path); >>> return -ENOMEM; >>> } >>> - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); >>> + >>> + if (recursive) { >>> + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | >>> FTW_PHYS); >>> + } else { >>> + ret = rmdir(dpath); >>> + if (ret) >>> + ERROR("Remove directory %s failed: %s", dpath, >>> strerror(errno)); >>> + } >>> + >>> free(dpath); >>> return ret; >>> } >>> diff --git a/include/util.h b/include/util.h >>> index 3c2a42f..9c21949 100644 >>> --- a/include/util.h >>> +++ b/include/util.h >>> @@ -258,7 +258,7 @@ const char* get_tmpdirscripts(void); >>> void swupdate_create_directory(const char* path); >>> #ifndef CONFIG_NOCLEANUP >>> -int swupdate_remove_directory(const char* path); >>> +int swupdate_remove_directory(const char* path, bool recursive); >>> #endif >>> int swupdate_mount(const char *device, const char *dir, const char >>> *fstype); >> >
On 04.09.2023 11:58, Stefano Babic wrote: > Hi Lukas, > > On 04.09.23 11:45, Lukas Funke wrote: >> Hi Stefano, >> >> On 04.09.2023 11:15, Stefano Babic wrote: >>> Hi Lukas, >>> >>> On 04.09.23 10:45, lukas.funke-oss@weidmueller.com wrote: >>>> From: Lukas Funke <lukas.funke@weidmueller.com> >>>> >>>> Files in DATADST_DIR belong to a mount point. The device >>>> is usually unmounted from this mountpoint and the directory >>>> is expected to be empty. However, in case of an error the >>>> mount may still exist. Then there could be files in the >>>> DATADST_DIR folder. Removing the files non-recursivly should then >>>> fail if the directory is non-empty. >>>> >>> >>> No. >>> >>> The DATADST_DIR is thought to be a temporary directory where >>> artifacts are extracted (if streaming is deactivated). The life of >>> files in DATADST_DIR should not survive an update session. After >>> SWUpdate has finished, data should be removed to avoid any possible >>> memory between further updates. >>> >>> Think also that letting these files could be (is it ? ) a security >>> leak. In fact, it could be possible to install something in >>> DATADST_DIR during a broken update, and using them in some way later. >>> >>> I imagine which is your use case, but this is the wrong approach. >>> SWUpdate mount / unmount during an upüdate to let the system in a >>> clean state. If unmount fails, someone in between is starting to >>> access the files causing the filesystem to be busy, but this is not >>> allowed until the update is declared successful. >>> >>> This patch will be rejected. >> >> Thanks for the clarification. The idea behind the patch is, if someone >> installs a bootloader using this way and - for whatever reason - the >> unmount fails, > > Well, putting the bootloader on a filesystem is *always* a bad idea. > There just a few cases where this is required (UEFI on EFI partition), > else it is safer to store it where the vendor suggests on raw flash. Agree :) > > Which is the reason the umount at that point fails ? It is seen as > transaction: > > swupdate_mount(); > > .....install anything > > umount(); > > And SWUpdate is designed to be sync with any handler (no concurrency of > handlers). If any process opens a file handle on that mountpoint (i.e. scanning for files or whatever), then the mountpoint will persist, because umount fails silently. At least a warning would be nice. If using the streaming interface the 'swupdate_remove_directory()' method will face a non-empty mounted directory on a failed umount and will remove it recursivly. Which is bad, I think. It's safe to just remove the directory because it 'should' always be empty, but it's the recursive part I've my concerns. > >> then the files in DATADST_DIR should not be removed, > > This happens with any artifact, but if umount() fails, it is supposed > something is going wrong. When does it happen on your site ? > > The only improvement I see is that handlers can use a new temporary > mount point instead of reusing DATADST_DIR, as the scope / goal is > different. > >> because then the bootloader would be removed, resulting in a bricked >> system. >> > > Best regards, > Stefano Babic > > >>> >>>> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com> >>>> --- >>>> core/stream_interface.c | 4 ++-- >>>> core/util.c | 12 ++++++++++-- >>>> include/util.h | 2 +- >>>> 3 files changed, 13 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/core/stream_interface.c b/core/stream_interface.c >>>> index 3f0b952..e03fc38 100644 >>>> --- a/core/stream_interface.c >>>> +++ b/core/stream_interface.c >>>> @@ -696,8 +696,8 @@ void *network_initializer(void *data) >>>> cleanup_files(software); >>>> #ifndef CONFIG_NOCLEANUP >>>> - swupdate_remove_directory(SCRIPTS_DIR_SUFFIX); >>>> - swupdate_remove_directory(DATADST_DIR_SUFFIX); >>>> + swupdate_remove_directory(SCRIPTS_DIR_SUFFIX, true); >>>> + swupdate_remove_directory(DATADST_DIR_SUFFIX, false); >>>> #endif >>>> pthread_mutex_lock(&stream_mutex); >>>> diff --git a/core/util.c b/core/util.c >>>> index a6ddf82..af22590 100644 >>>> --- a/core/util.c >>>> +++ b/core/util.c >>>> @@ -172,7 +172,7 @@ static int _remove_directory_cb(const char >>>> *fpath, const struct stat *sb, >>>> return remove(fpath); >>>> } >>>> -int swupdate_remove_directory(const char* path) >>>> +int swupdate_remove_directory(const char* path, bool recursive) >>>> { >>>> char* dpath; >>>> int ret; >>>> @@ -181,7 +181,15 @@ int swupdate_remove_directory(const char* path) >>>> ERROR("OOM: Directory %s not removed", path); >>>> return -ENOMEM; >>>> } >>>> - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); >>>> + >>>> + if (recursive) { >>>> + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | >>>> FTW_PHYS); >>>> + } else { >>>> + ret = rmdir(dpath); >>>> + if (ret) >>>> + ERROR("Remove directory %s failed: %s", dpath, >>>> strerror(errno)); >>>> + } >>>> + >>>> free(dpath); >>>> return ret; >>>> } >>>> diff --git a/include/util.h b/include/util.h >>>> index 3c2a42f..9c21949 100644 >>>> --- a/include/util.h >>>> +++ b/include/util.h >>>> @@ -258,7 +258,7 @@ const char* get_tmpdirscripts(void); >>>> void swupdate_create_directory(const char* path); >>>> #ifndef CONFIG_NOCLEANUP >>>> -int swupdate_remove_directory(const char* path); >>>> +int swupdate_remove_directory(const char* path, bool recursive); >>>> #endif >>>> int swupdate_mount(const char *device, const char *dir, const char >>>> *fstype); >>> >> >
Hi Lukas, On 04.09.23 12:39, Lukas Funke wrote: > On 04.09.2023 11:58, Stefano Babic wrote: >> Hi Lukas, >> >> On 04.09.23 11:45, Lukas Funke wrote: >>> Hi Stefano, >>> >>> On 04.09.2023 11:15, Stefano Babic wrote: >>>> Hi Lukas, >>>> >>>> On 04.09.23 10:45, lukas.funke-oss@weidmueller.com wrote: >>>>> From: Lukas Funke <lukas.funke@weidmueller.com> >>>>> >>>>> Files in DATADST_DIR belong to a mount point. The device >>>>> is usually unmounted from this mountpoint and the directory >>>>> is expected to be empty. However, in case of an error the >>>>> mount may still exist. Then there could be files in the >>>>> DATADST_DIR folder. Removing the files non-recursivly should then >>>>> fail if the directory is non-empty. >>>>> >>>> >>>> No. >>>> >>>> The DATADST_DIR is thought to be a temporary directory where >>>> artifacts are extracted (if streaming is deactivated). The life of >>>> files in DATADST_DIR should not survive an update session. After >>>> SWUpdate has finished, data should be removed to avoid any possible >>>> memory between further updates. >>>> >>>> Think also that letting these files could be (is it ? ) a security >>>> leak. In fact, it could be possible to install something in >>>> DATADST_DIR during a broken update, and using them in some way later. >>>> >>>> I imagine which is your use case, but this is the wrong approach. >>>> SWUpdate mount / unmount during an upüdate to let the system in a >>>> clean state. If unmount fails, someone in between is starting to >>>> access the files causing the filesystem to be busy, but this is not >>>> allowed until the update is declared successful. >>>> >>>> This patch will be rejected. >>> >>> Thanks for the clarification. The idea behind the patch is, if >>> someone installs a bootloader using this way and - for whatever >>> reason - the unmount fails, >> >> Well, putting the bootloader on a filesystem is *always* a bad idea. >> There just a few cases where this is required (UEFI on EFI partition), >> else it is safer to store it where the vendor suggests on raw flash. > > Agree :) > >> >> Which is the reason the umount at that point fails ? It is seen as >> transaction: >> >> swupdate_mount(); >> >> .....install anything >> >> umount(); >> >> And SWUpdate is designed to be sync with any handler (no concurrency >> of handlers). > > If any process opens a file handle on that mountpoint (i.e. scanning for > files or whatever), then the mountpoint will persist, because umount > fails silently. See above, processes are not allowed to do this. This is private data for SWUpdate, as well as UDS for communicationm. A process can also remove files that belongs to another process (see systemd that decides itself to drop files..), but it is then not supposed that the involved process could still run. This is not a use case. If this is becoming an issue, it should be solved in another way and permissions should be given (by setting uid / gid) so that the scanner is not allow to access files and DATADST_DIR. This is private data for SWUpdate. > At least a warning would be nice. A warning is ok. > > If using the streaming interface the 'swupdate_remove_directory()' > method will face a non-empty mounted directory on a failed umount and it face a not-empty directory if streaming is turned off. It should clean up. > and > will remove it recursivly. Which is bad, I think. No, it is not. There shouldn't be memory of previous update. A new update should run like it was the first update. If there are rests of previous updates, it is not. > It's safe to just > remove the directory because it 'should' always be empty, but it's the > recursive part I've my concerns. It shouldn't be empty, it depends on use cases. In your case, it is always empty when everything is ok. In other use cases, it is not and should be cleaned up. > >> >>> then the files in DATADST_DIR should not be removed, >> >> This happens with any artifact, but if umount() fails, it is supposed >> something is going wrong. When does it happen on your site ? >> >> The only improvement I see is that handlers can use a new temporary >> mount point instead of reusing DATADST_DIR, as the scope / goal is >> different. Note: this can be done because DATADST_DIR is used for multiple purposes, but removing recursively is ok. >> >>> because then the bootloader would be removed, resulting in a bricked >>> system. >>> >> Best regards, Stefano Babic >> >> >>>> >>>>> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com> >>>>> --- >>>>> core/stream_interface.c | 4 ++-- >>>>> core/util.c | 12 ++++++++++-- >>>>> include/util.h | 2 +- >>>>> 3 files changed, 13 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/core/stream_interface.c b/core/stream_interface.c >>>>> index 3f0b952..e03fc38 100644 >>>>> --- a/core/stream_interface.c >>>>> +++ b/core/stream_interface.c >>>>> @@ -696,8 +696,8 @@ void *network_initializer(void *data) >>>>> cleanup_files(software); >>>>> #ifndef CONFIG_NOCLEANUP >>>>> - swupdate_remove_directory(SCRIPTS_DIR_SUFFIX); >>>>> - swupdate_remove_directory(DATADST_DIR_SUFFIX); >>>>> + swupdate_remove_directory(SCRIPTS_DIR_SUFFIX, true); >>>>> + swupdate_remove_directory(DATADST_DIR_SUFFIX, false); >>>>> #endif >>>>> pthread_mutex_lock(&stream_mutex); >>>>> diff --git a/core/util.c b/core/util.c >>>>> index a6ddf82..af22590 100644 >>>>> --- a/core/util.c >>>>> +++ b/core/util.c >>>>> @@ -172,7 +172,7 @@ static int _remove_directory_cb(const char >>>>> *fpath, const struct stat *sb, >>>>> return remove(fpath); >>>>> } >>>>> -int swupdate_remove_directory(const char* path) >>>>> +int swupdate_remove_directory(const char* path, bool recursive) >>>>> { >>>>> char* dpath; >>>>> int ret; >>>>> @@ -181,7 +181,15 @@ int swupdate_remove_directory(const char* path) >>>>> ERROR("OOM: Directory %s not removed", path); >>>>> return -ENOMEM; >>>>> } >>>>> - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | >>>>> FTW_PHYS); >>>>> + >>>>> + if (recursive) { >>>>> + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | >>>>> FTW_PHYS); >>>>> + } else { >>>>> + ret = rmdir(dpath); >>>>> + if (ret) >>>>> + ERROR("Remove directory %s failed: %s", dpath, >>>>> strerror(errno)); >>>>> + } >>>>> + >>>>> free(dpath); >>>>> return ret; >>>>> } >>>>> diff --git a/include/util.h b/include/util.h >>>>> index 3c2a42f..9c21949 100644 >>>>> --- a/include/util.h >>>>> +++ b/include/util.h >>>>> @@ -258,7 +258,7 @@ const char* get_tmpdirscripts(void); >>>>> void swupdate_create_directory(const char* path); >>>>> #ifndef CONFIG_NOCLEANUP >>>>> -int swupdate_remove_directory(const char* path); >>>>> +int swupdate_remove_directory(const char* path, bool recursive); >>>>> #endif >>>>> int swupdate_mount(const char *device, const char *dir, const >>>>> char *fstype); >>>> >>> >> >
On 04.09.2023 13:06, Stefano Babic wrote: > Hi Lukas, > > On 04.09.23 12:39, Lukas Funke wrote: >> On 04.09.2023 11:58, Stefano Babic wrote: >>> Hi Lukas, >>> >>> On 04.09.23 11:45, Lukas Funke wrote: >>>> Hi Stefano, >>>> >>>> On 04.09.2023 11:15, Stefano Babic wrote: >>>>> Hi Lukas, >>>>> >>>>> On 04.09.23 10:45, lukas.funke-oss@weidmueller.com wrote: >>>>>> From: Lukas Funke <lukas.funke@weidmueller.com> >>>>>> >>>>>> Files in DATADST_DIR belong to a mount point. The device >>>>>> is usually unmounted from this mountpoint and the directory >>>>>> is expected to be empty. However, in case of an error the >>>>>> mount may still exist. Then there could be files in the >>>>>> DATADST_DIR folder. Removing the files non-recursivly should then >>>>>> fail if the directory is non-empty. >>>>>> >>>>> >>>>> No. >>>>> >>>>> The DATADST_DIR is thought to be a temporary directory where >>>>> artifacts are extracted (if streaming is deactivated). The life of >>>>> files in DATADST_DIR should not survive an update session. After >>>>> SWUpdate has finished, data should be removed to avoid any possible >>>>> memory between further updates. >>>>> >>>>> Think also that letting these files could be (is it ? ) a security >>>>> leak. In fact, it could be possible to install something in >>>>> DATADST_DIR during a broken update, and using them in some way later. >>>>> >>>>> I imagine which is your use case, but this is the wrong approach. >>>>> SWUpdate mount / unmount during an upüdate to let the system in a >>>>> clean state. If unmount fails, someone in between is starting to >>>>> access the files causing the filesystem to be busy, but this is not >>>>> allowed until the update is declared successful. >>>>> >>>>> This patch will be rejected. >>>> >>>> Thanks for the clarification. The idea behind the patch is, if >>>> someone installs a bootloader using this way and - for whatever >>>> reason - the unmount fails, >>> >>> Well, putting the bootloader on a filesystem is *always* a bad idea. >>> There just a few cases where this is required (UEFI on EFI >>> partition), else it is safer to store it where the vendor suggests on >>> raw flash. >> >> Agree :) >> >>> >>> Which is the reason the umount at that point fails ? It is seen as >>> transaction: >>> >>> swupdate_mount(); >>> >>> .....install anything >>> >>> umount(); >>> >>> And SWUpdate is designed to be sync with any handler (no concurrency >>> of handlers). >> >> If any process opens a file handle on that mountpoint (i.e. scanning >> for files or whatever), then the mountpoint will persist, because >> umount fails silently. > > See above, processes are not allowed to do this. This is private data > for SWUpdate, as well as UDS for communicationm. A process can also > remove files that belongs to another process (see systemd that decides > itself to drop files..), but it is then not supposed that the involved > process could still run. > > This is not a use case. Thanks for the clarification! I'll send another patch version with a warning instead of an error for a failed unmount and will drop this path. Best regards Lukas > > If this is becoming an issue, it should be solved in another way and > permissions should be given (by setting uid / gid) so that the scanner > is not allow to access files and DATADST_DIR. This is private data for > SWUpdate. > >> At least a warning would be nice. > > A warning is ok. > >> >> If using the streaming interface the 'swupdate_remove_directory()' >> method will face a non-empty mounted directory on a failed umount > > and it face a not-empty directory if streaming is turned off. It should > clean up. > >> and will remove it recursivly. Which is bad, I think. > > No, it is not. There shouldn't be memory of previous update. A new > update should run like it was the first update. If there are rests of > previous updates, it is not. > >> It's safe to just remove the directory because it 'should' always be >> empty, but it's the recursive part I've my concerns. > > It shouldn't be empty, it depends on use cases. In your case, it is > always empty when everything is ok. In other use cases, it is not and > should be cleaned up. > >> >>> >>>> then the files in DATADST_DIR should not be removed, >>> >>> This happens with any artifact, but if umount() fails, it is supposed >>> something is going wrong. When does it happen on your site ? >>> >>> The only improvement I see is that handlers can use a new temporary >>> mount point instead of reusing DATADST_DIR, as the scope / goal is >>> different. > > Note: this can be done because DATADST_DIR is used for multiple > purposes, but removing recursively is ok. > >>> >>>> because then the bootloader would be removed, resulting in a bricked >>>> system. >>>> >>> > > Best regards, > Stefano Babic > > >>> >>> >>>>> >>>>>> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com> >>>>>> --- >>>>>> core/stream_interface.c | 4 ++-- >>>>>> core/util.c | 12 ++++++++++-- >>>>>> include/util.h | 2 +- >>>>>> 3 files changed, 13 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/core/stream_interface.c b/core/stream_interface.c >>>>>> index 3f0b952..e03fc38 100644 >>>>>> --- a/core/stream_interface.c >>>>>> +++ b/core/stream_interface.c >>>>>> @@ -696,8 +696,8 @@ void *network_initializer(void *data) >>>>>> cleanup_files(software); >>>>>> #ifndef CONFIG_NOCLEANUP >>>>>> - swupdate_remove_directory(SCRIPTS_DIR_SUFFIX); >>>>>> - swupdate_remove_directory(DATADST_DIR_SUFFIX); >>>>>> + swupdate_remove_directory(SCRIPTS_DIR_SUFFIX, true); >>>>>> + swupdate_remove_directory(DATADST_DIR_SUFFIX, false); >>>>>> #endif >>>>>> pthread_mutex_lock(&stream_mutex); >>>>>> diff --git a/core/util.c b/core/util.c >>>>>> index a6ddf82..af22590 100644 >>>>>> --- a/core/util.c >>>>>> +++ b/core/util.c >>>>>> @@ -172,7 +172,7 @@ static int _remove_directory_cb(const char >>>>>> *fpath, const struct stat *sb, >>>>>> return remove(fpath); >>>>>> } >>>>>> -int swupdate_remove_directory(const char* path) >>>>>> +int swupdate_remove_directory(const char* path, bool recursive) >>>>>> { >>>>>> char* dpath; >>>>>> int ret; >>>>>> @@ -181,7 +181,15 @@ int swupdate_remove_directory(const char* path) >>>>>> ERROR("OOM: Directory %s not removed", path); >>>>>> return -ENOMEM; >>>>>> } >>>>>> - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | >>>>>> FTW_PHYS); >>>>>> + >>>>>> + if (recursive) { >>>>>> + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | >>>>>> FTW_PHYS); >>>>>> + } else { >>>>>> + ret = rmdir(dpath); >>>>>> + if (ret) >>>>>> + ERROR("Remove directory %s failed: %s", dpath, >>>>>> strerror(errno)); >>>>>> + } >>>>>> + >>>>>> free(dpath); >>>>>> return ret; >>>>>> } >>>>>> diff --git a/include/util.h b/include/util.h >>>>>> index 3c2a42f..9c21949 100644 >>>>>> --- a/include/util.h >>>>>> +++ b/include/util.h >>>>>> @@ -258,7 +258,7 @@ const char* get_tmpdirscripts(void); >>>>>> void swupdate_create_directory(const char* path); >>>>>> #ifndef CONFIG_NOCLEANUP >>>>>> -int swupdate_remove_directory(const char* path); >>>>>> +int swupdate_remove_directory(const char* path, bool recursive); >>>>>> #endif >>>>>> int swupdate_mount(const char *device, const char *dir, const >>>>>> char *fstype); >>>>> >>>> >>> >> >
diff --git a/core/stream_interface.c b/core/stream_interface.c index 3f0b952..e03fc38 100644 --- a/core/stream_interface.c +++ b/core/stream_interface.c @@ -696,8 +696,8 @@ void *network_initializer(void *data) cleanup_files(software); #ifndef CONFIG_NOCLEANUP - swupdate_remove_directory(SCRIPTS_DIR_SUFFIX); - swupdate_remove_directory(DATADST_DIR_SUFFIX); + swupdate_remove_directory(SCRIPTS_DIR_SUFFIX, true); + swupdate_remove_directory(DATADST_DIR_SUFFIX, false); #endif pthread_mutex_lock(&stream_mutex); diff --git a/core/util.c b/core/util.c index a6ddf82..af22590 100644 --- a/core/util.c +++ b/core/util.c @@ -172,7 +172,7 @@ static int _remove_directory_cb(const char *fpath, const struct stat *sb, return remove(fpath); } -int swupdate_remove_directory(const char* path) +int swupdate_remove_directory(const char* path, bool recursive) { char* dpath; int ret; @@ -181,7 +181,15 @@ int swupdate_remove_directory(const char* path) ERROR("OOM: Directory %s not removed", path); return -ENOMEM; } - ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); + + if (recursive) { + ret = nftw(dpath, _remove_directory_cb, 64, FTW_DEPTH | FTW_PHYS); + } else { + ret = rmdir(dpath); + if (ret) + ERROR("Remove directory %s failed: %s", dpath, strerror(errno)); + } + free(dpath); return ret; } diff --git a/include/util.h b/include/util.h index 3c2a42f..9c21949 100644 --- a/include/util.h +++ b/include/util.h @@ -258,7 +258,7 @@ const char* get_tmpdirscripts(void); void swupdate_create_directory(const char* path); #ifndef CONFIG_NOCLEANUP -int swupdate_remove_directory(const char* path); +int swupdate_remove_directory(const char* path, bool recursive); #endif int swupdate_mount(const char *device, const char *dir, const char *fstype);