Hello Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/49190
to review the following change.
Change subject: cbfstool: Use flock() when accessing CBFS files ......................................................................
cbfstool: Use flock() when accessing CBFS files
Trying to do multiple operations on the same CBFS image at the same time likely leads to data corruption. For this reason, add BSD advisory file locking (flock()) to cbfstool (and ifittool which is using the same file I/O library), so that only one process will operate on the same file at the same time and the others will wait in line. This should help resolve parallel build issues with the INTERMEDIATE target on certain platforms.
Unfortunately, some platforms use the INTERMEDIATE target to do a direct dd into the CBFS image. This should generally be discouraged and future platforms should aim to clearly deliminate regions that need to be written directly by platform scripts with custom FMAP sections, so that they can be written with `cbfstool write`. For the time being, update the legacy platforms that do this with explicit calls to the `flock` utility.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I022468f6957415ae68a7a7e70428ae6f82d23b06 --- M src/ec/hp/kbc1126/Makefile.inc M src/soc/amd/stoneyridge/Makefile.inc M src/southbridge/amd/pi/hudson/Makefile.inc M src/southbridge/intel/common/firmware/Makefile.inc M util/cbfstool/partitioned_file.c 5 files changed, 8 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/49190/1
diff --git a/src/ec/hp/kbc1126/Makefile.inc b/src/ec/hp/kbc1126/Makefile.inc index 43e5d3f..d767bad 100644 --- a/src/ec/hp/kbc1126/Makefile.inc +++ b/src/ec/hp/kbc1126/Makefile.inc @@ -22,7 +22,7 @@ printf " Building kbc1126_ec_insert.\n" $(MAKE) -C util/kbc1126 printf " KBC1126 Inserting KBC1126 firmware blobs.\n" - $(KBC1126_EC_INSERT) $(obj)/coreboot.pre \ + flock $< $(KBC1126_EC_INSERT) $(obj)/coreboot.pre \ $(CONFIG_KBC1126_FW1_OFFSET) $(CONFIG_KBC1126_FW2_OFFSET) endif
diff --git a/src/soc/amd/stoneyridge/Makefile.inc b/src/soc/amd/stoneyridge/Makefile.inc index 969f512..2ad0730 100644 --- a/src/soc/amd/stoneyridge/Makefile.inc +++ b/src/soc/amd/stoneyridge/Makefile.inc @@ -172,7 +172,7 @@ add_amdfw: $(obj)/coreboot.pre $(obj)/amdfw.rom printf " DD Adding AMD Firmware at ROM offset 0x%x\n" \ "$(STONEYRIDGE_FWM_ROM_POSITION)" - dd if=$(obj)/amdfw.rom \ + flock $< dd if=$(obj)/amdfw.rom \ of=$(obj)/coreboot.pre conv=notrunc bs=1 \ seek=$(STONEYRIDGE_FWM_ROM_POSITION) >/dev/null 2>&1
diff --git a/src/southbridge/amd/pi/hudson/Makefile.inc b/src/southbridge/amd/pi/hudson/Makefile.inc index 215a5a1..5eac024 100644 --- a/src/southbridge/amd/pi/hudson/Makefile.inc +++ b/src/southbridge/amd/pi/hudson/Makefile.inc @@ -162,7 +162,7 @@
add_amdfw: $(obj)/coreboot.pre $(obj)/amdfw.rom printf " DD Adding AMD Firmware\n" - dd if=$(obj)/amdfw.rom \ + flock $< dd if=$(obj)/amdfw.rom \ of=$(obj)/coreboot.pre conv=notrunc bs=1 seek=131072 >/dev/null 2>&1
else # ifeq ($(CONFIG_AMDFW_OUTSIDE_CBFS),y) diff --git a/src/southbridge/intel/common/firmware/Makefile.inc b/src/southbridge/intel/common/firmware/Makefile.inc index 516cd4d..588c663 100644 --- a/src/southbridge/intel/common/firmware/Makefile.inc +++ b/src/southbridge/intel/common/firmware/Makefile.inc @@ -35,7 +35,7 @@ endif add_intel_firmware: $(obj)/coreboot.pre $(IFDTOOL) printf " DD Adding Intel Firmware Descriptor\n" - dd if=$(IFD_BIN_PATH) \ + flock $< dd if=$(IFD_BIN_PATH) \ of=$(obj)/coreboot.pre conv=notrunc >/dev/null 2>&1 ifeq ($(CONFIG_VALIDATE_INTEL_DESCRIPTOR),y) $(objutil)/ifdtool/ifdtool \ diff --git a/util/cbfstool/partitioned_file.c b/util/cbfstool/partitioned_file.c index b6d4f1b..6e75600 100644 --- a/util/cbfstool/partitioned_file.c +++ b/util/cbfstool/partitioned_file.c @@ -8,6 +8,7 @@ #include <assert.h> #include <stdlib.h> #include <string.h> +#include <sys/file.h>
struct partitioned_file { struct fmap *fmap; @@ -57,7 +58,7 @@ access_mode = write_access ? "rb+" : "rb"; file->stream = fopen(filename, access_mode);
- if (!file->stream) { + if (!file->stream || flock(fileno(file->stream), LOCK_EX)) { perror(filename); partitioned_file_close(file); return NULL; @@ -78,7 +79,7 @@ }
file->stream = fopen(filename, "wb"); - if (!file->stream) { + if (!file->stream || flock(fileno(file->stream), LOCK_EX)) { perror(filename); free(file); return NULL; @@ -268,6 +269,7 @@ file->fmap = NULL; buffer_delete(&file->buffer); if (file->stream) { + flock(fileno(file->stream), LOCK_UN); fclose(file->stream); file->stream = NULL; }
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49190 )
Change subject: cbfstool: Use flock() when accessing CBFS files ......................................................................
Patch Set 1: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49190 )
Change subject: cbfstool: Use flock() when accessing CBFS files ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/49190 )
Change subject: cbfstool: Use flock() when accessing CBFS files ......................................................................
cbfstool: Use flock() when accessing CBFS files
Trying to do multiple operations on the same CBFS image at the same time likely leads to data corruption. For this reason, add BSD advisory file locking (flock()) to cbfstool (and ifittool which is using the same file I/O library), so that only one process will operate on the same file at the same time and the others will wait in line. This should help resolve parallel build issues with the INTERMEDIATE target on certain platforms.
Unfortunately, some platforms use the INTERMEDIATE target to do a direct dd into the CBFS image. This should generally be discouraged and future platforms should aim to clearly deliminate regions that need to be written directly by platform scripts with custom FMAP sections, so that they can be written with `cbfstool write`. For the time being, update the legacy platforms that do this with explicit calls to the `flock` utility.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I022468f6957415ae68a7a7e70428ae6f82d23b06 Reviewed-on: https://review.coreboot.org/c/coreboot/+/49190 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Furquan Shaikh furquan@google.com --- M src/ec/hp/kbc1126/Makefile.inc M src/soc/amd/stoneyridge/Makefile.inc M src/southbridge/amd/pi/hudson/Makefile.inc M src/southbridge/intel/common/firmware/Makefile.inc M util/cbfstool/partitioned_file.c 5 files changed, 8 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Arthur Heymans: Looks good to me, but someone else must approve
diff --git a/src/ec/hp/kbc1126/Makefile.inc b/src/ec/hp/kbc1126/Makefile.inc index 43e5d3f..d767bad 100644 --- a/src/ec/hp/kbc1126/Makefile.inc +++ b/src/ec/hp/kbc1126/Makefile.inc @@ -22,7 +22,7 @@ printf " Building kbc1126_ec_insert.\n" $(MAKE) -C util/kbc1126 printf " KBC1126 Inserting KBC1126 firmware blobs.\n" - $(KBC1126_EC_INSERT) $(obj)/coreboot.pre \ + flock $< $(KBC1126_EC_INSERT) $(obj)/coreboot.pre \ $(CONFIG_KBC1126_FW1_OFFSET) $(CONFIG_KBC1126_FW2_OFFSET) endif
diff --git a/src/soc/amd/stoneyridge/Makefile.inc b/src/soc/amd/stoneyridge/Makefile.inc index 969f512..2ad0730 100644 --- a/src/soc/amd/stoneyridge/Makefile.inc +++ b/src/soc/amd/stoneyridge/Makefile.inc @@ -172,7 +172,7 @@ add_amdfw: $(obj)/coreboot.pre $(obj)/amdfw.rom printf " DD Adding AMD Firmware at ROM offset 0x%x\n" \ "$(STONEYRIDGE_FWM_ROM_POSITION)" - dd if=$(obj)/amdfw.rom \ + flock $< dd if=$(obj)/amdfw.rom \ of=$(obj)/coreboot.pre conv=notrunc bs=1 \ seek=$(STONEYRIDGE_FWM_ROM_POSITION) >/dev/null 2>&1
diff --git a/src/southbridge/amd/pi/hudson/Makefile.inc b/src/southbridge/amd/pi/hudson/Makefile.inc index 215a5a1..5eac024 100644 --- a/src/southbridge/amd/pi/hudson/Makefile.inc +++ b/src/southbridge/amd/pi/hudson/Makefile.inc @@ -162,7 +162,7 @@
add_amdfw: $(obj)/coreboot.pre $(obj)/amdfw.rom printf " DD Adding AMD Firmware\n" - dd if=$(obj)/amdfw.rom \ + flock $< dd if=$(obj)/amdfw.rom \ of=$(obj)/coreboot.pre conv=notrunc bs=1 seek=131072 >/dev/null 2>&1
else # ifeq ($(CONFIG_AMDFW_OUTSIDE_CBFS),y) diff --git a/src/southbridge/intel/common/firmware/Makefile.inc b/src/southbridge/intel/common/firmware/Makefile.inc index 516cd4d..588c663 100644 --- a/src/southbridge/intel/common/firmware/Makefile.inc +++ b/src/southbridge/intel/common/firmware/Makefile.inc @@ -35,7 +35,7 @@ endif add_intel_firmware: $(obj)/coreboot.pre $(IFDTOOL) printf " DD Adding Intel Firmware Descriptor\n" - dd if=$(IFD_BIN_PATH) \ + flock $< dd if=$(IFD_BIN_PATH) \ of=$(obj)/coreboot.pre conv=notrunc >/dev/null 2>&1 ifeq ($(CONFIG_VALIDATE_INTEL_DESCRIPTOR),y) $(objutil)/ifdtool/ifdtool \ diff --git a/util/cbfstool/partitioned_file.c b/util/cbfstool/partitioned_file.c index b6d4f1b..6e75600 100644 --- a/util/cbfstool/partitioned_file.c +++ b/util/cbfstool/partitioned_file.c @@ -8,6 +8,7 @@ #include <assert.h> #include <stdlib.h> #include <string.h> +#include <sys/file.h>
struct partitioned_file { struct fmap *fmap; @@ -57,7 +58,7 @@ access_mode = write_access ? "rb+" : "rb"; file->stream = fopen(filename, access_mode);
- if (!file->stream) { + if (!file->stream || flock(fileno(file->stream), LOCK_EX)) { perror(filename); partitioned_file_close(file); return NULL; @@ -78,7 +79,7 @@ }
file->stream = fopen(filename, "wb"); - if (!file->stream) { + if (!file->stream || flock(fileno(file->stream), LOCK_EX)) { perror(filename); free(file); return NULL; @@ -268,6 +269,7 @@ file->fmap = NULL; buffer_delete(&file->buffer); if (file->stream) { + flock(fileno(file->stream), LOCK_UN); fclose(file->stream); file->stream = NULL; }
-
Arthur Heymans (Code Review) -
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)