Re: [PATCH v4 2/4] vvfat: Fix usage of `info.file.offset`

[Amjad Alsharafi]

On Mon, Jun 10, 2024 at 06:49:43PM +0200, Kevin Wolf wrote:
> Am 05.06.2024 um 02:58 hat Amjad Alsharafi geschrieben:
> > The field is marked as "the offset in the file (in clusters)", but it
> > was being used like this
> > `cluster_size*(nums)+mapping->info.file.offset`, which is incorrect.
> > 
> > Additionally, removed the `abort` when `first_mapping_index` does not
> > match, as this matches the case when adding new clusters for files, and
> > its inevitable that we reach this condition when doing that if the
> > clusters are not after one another, so there is no reason to `abort`
> > here, execution continues and the new clusters are written to disk
> > correctly.
> > 
> > Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
> 
> Can you help me understand how first_mapping_index really works?
> 
> It seems to me that you get a chain of mappings for each file on the FAT
> filesystem, which are just the contiguous areas in it, and
> first_mapping_index refers to the mapping at the start of the file. But
> for much of the time, it actually doesn't seem to be set at all, so you
> have mapping->first_mapping_index == -1. Do you understand the rules
> around when it's set and when it isn't?

Yeah. So `first_mapping_index` is the index of the first mapping, each
mapping is a group of clusters that are contiguous in the file.
Its mostly `-1` because the first mapping will have the value set as
`-1` and not its own index, this value will only be set when the file
contain more than one mapping, and this will only happen when you add
clusters to a file that are not contiguous with the existing clusters.

And actually, thanks to that I noticed another bug not fixed in PATCH 3, 
We are doing this check 
`s->current_mapping->first_mapping_index != mapping->first_mapping_index`
to know if we should switch to the new mapping or not. 
If we were reading from the first mapping (`first_mapping_index == -1`)
and we jumped to the second mapping (`first_mapping_index == n`), we
will catch this condition and switch to the new mapping.

But if the file has more than 2 mappings, and we jumped to the 3rd
mapping, we will not catch this since (`first_mapping_index == n`) for
both of them haha. I think a better check is to check the `mapping`
pointer directly. (I'll add it also in the next series together with a
test for it.)

> 
> >  block/vvfat.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index 19da009a5b..f0642ac3e4 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -1408,7 +1408,9 @@ read_cluster_directory:
> >  
> >          assert(s->current_fd);
> >  
> > -        
> > offset=s->cluster_size*(cluster_num-s->current_mapping->begin)+s->current_mapping->info.file.offset;
> > +        offset = s->cluster_size *
> > +            ((cluster_num - s->current_mapping->begin)
> > +            + s->current_mapping->info.file.offset);
> >          if(lseek(s->current_fd, offset, SEEK_SET)!=offset)
> >              return -3;
> >          s->cluster=s->cluster_buffer;
> > @@ -1929,8 +1931,9 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, 
> > direntry_t* direntry, const ch
> >                          (mapping->mode & MODE_DIRECTORY) == 0) {
> >  
> >                      /* was modified in qcow */
> > -                    if (offset != mapping->info.file.offset + 
> > s->cluster_size
> > -                            * (cluster_num - mapping->begin)) {
> > +                    if (offset != s->cluster_size
> > +                            * ((cluster_num - mapping->begin)
> > +                            + mapping->info.file.offset)) {
> >                          /* offset of this cluster in file chain has 
> > changed */
> >                          abort();
> >                          copy_it = 1;
> > @@ -1944,7 +1947,6 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, 
> > direntry_t* direntry, const ch
> >  
> >                      if (mapping->first_mapping_index != first_mapping_index
> >                              && mapping->info.file.offset > 0) {
> > -                        abort();
> >                          copy_it = 1;
> >                      }
> 
> I'm unsure which case this represents. If first_mapping_index refers to
> the mapping of the first cluster in the file, does this mean we got a
> mapping for a different file here? Or is the comparison between -1 and a
> real value?

Now that I think more about it, I think this `abort` is actually
correct, the issue though is that the handling around this code is not.

What this `abort` actually does is that it checks.
- if the `mapping->first_mapping_index` is not the same as
  `first_mapping_index`, which **should** happen only in one case, when
  we are handling the first mapping, in that case
  `mapping->first_mapping_index == -1`, in all other cases, the other
  mappings after the first should have the condition true.
- From above, we know that this is the first mapping, so if the offset
  is not `0`, then abort, since this is an invalid state.

This is all good, the issue is that `first_mapping_index` is not set if
we are checking from the middle, the variable `first_mapping_index` is
only set if we passed through the check `cluster_was_modified` with the
first mapping, and in the same function call we checked the other
mappings.

>From what I have seen, that doesn't happen since even if you write the
whole file in one go, you are still writing it cluster by cluster, and
the checks happen at that time.

> 
> In any case it doesn't seem to be the case that the comment at the
> declaration of copy_it describes.
> 
> >  
> > @@ -2404,7 +2406,7 @@ static int commit_mappings(BDRVVVFATState* s,
> >                          (mapping->end - mapping->begin);
> >              } else
> >                  next_mapping->info.file.offset = mapping->info.file.offset 
> > +
> > -                        mapping->end - mapping->begin;
> > +                        (mapping->end - mapping->begin);
> >  
> >              mapping = next_mapping;
> >          }
> 
> Kevin
>