On Tue, Feb 12, 2019 at 04:41:47PM +0000, Mike Small wrote: > "Brian C. Lane" <b...@redhat.com> writes: > > > On Fri, Feb 08, 2019 at 11:03:55PM +0000, Mike Small wrote: > >> Hi, > >> > >> Someone shared with me a case where parted 3.2 (3.2-15 as packaged in > >> Ubuntu Xenial) hit a sigsegv when run as follows: > > > > Good job tracking this down! Yes, a test would be good to have, I think > > this is one of those corner cases that can bite people and lead to lots > > of confusion :) > > I'll start working on the tests today. Maybe I should try installing > nilfs on a partition and make sure that still works too after the patch > is in good shape.
That's probably a good idea. > > > > >> crc = __efi_crc32(sb, sumoff, PED_LE32_TO_CPU(sb->s_crc_seed)); > >> @@ -113,11 +113,13 @@ nilfs2_probe (PedGeometry* geom) > >> const int sectors = (4096 + geom->dev->sector_size - 1) / > >> geom->dev->sector_size; > >> char *buf = alloca (sectors * geom->dev->sector_size); > >> - void *buff2 = alloca (geom->dev->sector_size); > >> + const int sectors2 = sizeof(struct nilfs2_super_block) / > >> geom->dev->sector_size + > >> + (sizeof(struct nilfs2_super_block) % > >> geom->dev->sector_size == 0) ? 0 : 1; > > > > This calculation is correct, but I find it hard to read. If you use the > > same technique as it does for sectors it would be easier to understand > > in the future, and I don't think the superblock size is going to change. > > Probably I should have spent more time trying to understand the way > sectors was calculated or asked about it before submitting the patch. It > confused me, since in my case, where geom->dev->sector_size was 512, > that calculation gave a size that meant eight 512 byte sectors were read > instead of two (sizeof nilfs2_super_block = 1024): > > (4096 + 512 - 1) / 512 = 8. > > And that's what it did, except all at once, based on the strace... > > lseek(3, 11813257216, SEEK_SET) = 11813257216 > read(3, > "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 4096) > = 4096 > > And then there was the 1024 offset introduced when assigning to the > primary superblock, sb, which I didn't understand the purpose of... > > if (ped_geometry_read(geom, buf, 0, sectors)) > sb = (struct nilfs2_super_block *)(buf+1024); > > > I wasn't sure if reading the extra six sectors for the 2nd superblock > would be okay, e.g. if the superblock was really close to the end of a > disk. And in general there are these things about reading the first > superblock which I don't understand, so I'm unclear if the two lengths > should be computed the same way. If so should we look for the 2nd > superblock 1024 bytes into the 4096 bytes read like we do for the 1st > superblock? I can't seem to find a decent reference for NILFS other than this code and the linux kernel code so I'm not sure why it reads so much for the first one. I think you've got the logic right, I just think it would be easier to read as: sectors2 = (1024 + geom->dev->sector_size - 1) / geom->dev->sector_size; When reading the 2nd superblock it looks like it starts on a sector boundary so that's why it doesn't need the 4096 offset. -- Brian C. Lane (PST8PDT)