Re: [PATCH bpf-next v10 06/10] bpf,landlock: Add a new map type: inode

[Alexei Starovoitov]

On Wed, Jul 31, 2019 at 09:11:10PM +0200, MickaÃl SalaÃn wrote:
> 
> 
> On 31/07/2019 20:58, Alexei Starovoitov wrote:
> > On Wed, Jul 31, 2019 at 11:46 AM MickaÃl SalaÃn
> > <mickael.salaun@xxxxxxxxxxx> wrote:
> >>>> +    for (i = 0; i < htab->n_buckets; i++) {
> >>>> +            head = select_bucket(htab, i);
> >>>> +            hlist_nulls_for_each_entry_safe(l, n, head, hash_node) {
> >>>> +                    landlock_inode_remove_map(*((struct inode **)l->key), map);
> >>>> +            }
> >>>> +    }
> >>>> +    htab_map_free(map);
> >>>> +}
> >>>
> >>> user space can delete the map.
> >>> that will trigger inode_htab_map_free() which will call
> >>> landlock_inode_remove_map().
> >>> which will simply itereate the list and delete from the list.
> >>
> >> landlock_inode_remove_map() removes the reference to the map (being
> >> freed) from the inode (with an RCU lock).
> >
> > I'm going to ignore everything else for now and focus only on this bit,
> > since it's fundamental issue to address before this discussion can
> > go any further.
> > rcu_lock is not a spin_lock. I'm pretty sure you know this.
> > But you're arguing that it's somehow protecting from the race
> > I mentioned above?
> >
> 
> I was just clarifying your comment to avoid misunderstanding about what
> is being removed.
> 
> As said in the full response, there is currently a race but, if I add a
> bpf_map_inc() call when the map is referenced by inode->security, then I
> don't see how a race could occur because such added map could only be
> freed in a security_inode_free() (as long as it retains a reference to
> this inode).

then it will be a cycle and a map will never be deleted?
closing map_fd should delete a map. It cannot be alive if it's not
pinned in bpffs, there are no FDs that are holding it, and no progs using it.
So the map deletion will iterate over inodes that belong to this map.
In parallel security_inode_free() will be called that will iterate
over its link list that contains elements from different maps.
So the same link list is modified by two cpus.
Where is a lock that protects from concurrent links list manipulations?

> Les donnÃes à caractÃre personnel recueillies et traitÃes dans le cadre de cet Ãchange, le sont à seule fin dâexÃcution dâune relation professionnelle et sâopÃrent dans cette seule finalità et pour la durÃe nÃcessaire à cette relation. Si vous souhaitez faire usage de vos droits de consultation, de rectification et de suppression de vos donnÃes, veuillez contacter contact.rgpd@xxxxxxxxxxxxxx Si vous avez reÃu ce message par erreur, nous vous remercions dâen informer lâexpÃditeur et de dÃtruire le message. The personal data collected and processed during this exchange aims solely at completing a business relationship and is limited to the necessary duration of that relationship. If you wish to use your rights of consultation, rectification and deletion of your data, please contact: contact.rgpd@xxxxxxxxxxxxxx If you have received this message in error, we thank you for informing the sender and destroying the message.

Please get rid of this. It's absolutely not appropriate on public mailing list.
Next time I'd have to ignore emails that contain such disclaimers.