[CRIU] [PATCH v8 00/15] AutoFS mounts migration support
Stanislav Kinsburskiy
skinsbursky at virtuozzo.com
Thu Mar 24 04:32:37 PDT 2016
Thanks.
Series "autofs: fix issues, reported by coverity" address these issues.
24.03.2016 00:03, Andrew Vagin пишет:
> Stas, here are four issues which were reported by Coverirty:
>
> 324static int autofs_dump_entry(struct mount_info *pm, AutofsEntry
> *entry)
> 325{
> 326 struct cr_img *img;
> 327 int ret = -1;
> 328
> 329 img = open_image(CR_FD_AUTOFS, O_DUMP, pm->k_dev);
> 1. Condition img, taking false branch
> 2. var_compare_op: Comparing img to null implies that img might
> be null.
> 330 if (img)
> 331 ret = pb_write_one(img, entry, PB_AUTOFS);
>
> CID 158925 (#1 of 1): Dereference after null check (FORWARD_NULL)
> 3. var_deref_model: Passing null pointer img to close_image, which
> dereferences it. [show details]
> 332 close_image(img);
> 333 return ret;
> 334}
>
>
> 337int autofs_dump(struct mount_info *pm)
> 338{
> 339 AutofsEntry *entry;
> 340
> 1. alloc_fn: Storage is returned from allocation function
> malloc.
> 2. var_assign: Assigning: ___p = storage returned from
> malloc(72UL).
> 3. Condition !___p, taking false branch
> 4. leaked_storage: Variable ___p going out of scope leaks the
> storage it points to.
> 5. var_assign: Assigning: entry = ({...}).
> 341 entry = xmalloc(sizeof(*entry));
> 6. Condition !entry, taking false branch
> 342 if (!entry)
> 343 return -1;
> 7. noescape: Resource entry is not freed or pointed-to in
> autofs_entry__init. [show details]
> 344 autofs_entry__init(entry);
> 345
> 8. noescape: Resource entry is not freed or pointed-to in
> autofs_create_entry. [show details]
> 9. Condition autofs_create_entry(pm, entry), taking true branch
> 346 if (autofs_create_entry(pm, entry))
>
> CID 158926 (#1 of 1): Resource leak (RESOURCE_LEAK)
> 10. leaked_storage: Variable entry going out of scope leaks the storage
> it points to.
> 347 return -1;
> 348
> 349 return autofs_dump_entry(pm, entry);
> 350}
>
> int autofs_mount(struct mount_info *mi, const char *source, const
> 822 char *filesystemtype, unsigned long mountflags)
> 823{
> 824 AutofsEntry *entry;
> 825 autofs_info_t *info;
> 826 char *opts, *mode;
> 827 int control_pipe[2], ret = -1;
> 828 struct stat buf;
> 829
> 830 if (autofs_restore_entry(mi, &entry) < 0)
> 831 return -1;
> 832
> 833 if (pipe(control_pipe) < 0) {
> 834 pr_perror("Can't create pipe");
> 835 return -1;
> 836 }
> 837
> 838 mode = "direct";
> 839 if (entry->mode == AUTOFS_MODE_INDIRECT)
> 840 mode = "indirect";
> 1. Starting defect path here.
> 2. Condition entry->mode == 2, taking true branch
> 841 if (entry->mode == AUTOFS_MODE_OFFSET)
> 842 mode = "offset";
> 843
> 844 opts = xsprintf("fd=%d,pgrp=%d,minproto=%d,maxproto=%d,%s",
> 845 control_pipe[1], getpgrp(), entry->minproto,
> 846 entry->maxproto, mode);
> 3. Condition opts, taking true branch
> 4. Condition entry->has_uid, taking true branch
> 847 if (opts && entry->has_uid)
> 848 opts = xstrcat(opts, ",uid=%d", entry->uid);
> 5. Condition opts, taking true branch
> 6. Condition entry->has_gid, taking true branch
> 849 if (opts && entry->has_gid)
> 850 opts = xstrcat(opts, ",gid=%d", entry->gid);
> 7. Condition !opts, taking false branch
> 851 if (!opts) {
> 852 pr_err("Failed to create options string\n");
> 853 goto close_pipe;
> 854 }
> 855
> 856 pr_info("autofs: mounting to %s with options: \"%s\"\n",
> 857 mi->mountpoint, opts);
> 858
> 8. Condition mount(source, mi->mountpoint, filesystemtype,
> mountflags, opts) < 0, taking false branch
> 859 if (mount(source, mi->mountpoint, filesystemtype, mountflags,
> opts) < 0) {
> 860 pr_perror("Failed to mount autofs to %s",
> mi->mountpoint);
> 861 goto close_pipe;
> 862 }
> 863
> 9. alloc_fn: Storage is returned from allocation function
> malloc.
> 10. var_assign: Assigning: ___p = storage returned from
> malloc(120UL).
> 11. Condition !___p, taking false branch
> 12. leaked_storage: Variable ___p going out of scope leaks the
> storage it points to.
> 13. var_assign: Assigning: info = ({...}).
> 864 info = xmalloc(sizeof(*info));
> 14. Condition !info, taking false branch
> 865 if (!info)
> 866 goto umount;
> 867 info->entry = entry;
> 868
> 869 /* We need autofs dev_id to be able to open direct mount
> point.
> 870 * But we can't call stat in autofs_add_mount_info(), because
> autofs
> 871 * mount can be overmounted. Thus we have to call it here.
> But shared
> 872 * data is not ready yet. So, let's put in on mi->private and
> copy to
> 873 * shared data in autofs_add_mount_info().
> 874 */
> CID 158928: Time of check time of use (TOCTOU) [select issue]
> 15. Condition stat(mi->mountpoint, &buf) < 0, taking false
> branch
> 875 if (stat(mi->mountpoint, &buf) < 0) {
> 876 pr_perror("Failed to stat %s", mi->mountpoint);
> 877 goto umount;
> 878 }
> 879 info->mnt_dev = buf.st_dev;
> 880
> 881 /* We need to create dentries for nested mounts */
> 882 ret = autofs_populate_mount(mi, entry);
> 16. Condition ret < 0, taking false branch
> 883 if (ret < 0)
> 884 goto umount;
> 885
> 886 /* In case of catatonic mounts all we need as the function
> call below */
> 887 ret = autofs_post_mount(mi->mountpoint, buf.st_dev,
> entry->timeout);
> 17. Condition ret < 0, taking false branch
> 888 if (ret < 0)
> 889 goto umount;
> 890
> 891 /* Otherwise we have to add shared object creation callback
> */
> 18. Condition entry->fd != -1, taking true branch
> 892 if (entry->fd != AUTOFS_CATATONIC_FD) {
> 893 ret = add_post_prepare_cb(autofs_add_mount_info, mi);
> 19. Condition ret < 0, taking true branch
> 894 if (ret < 0)
> 20. Jumping to label umount
> 895 goto umount;
> 896 }
> 897
> 898 mi->private = info;
> 899
> 900close_pipe:
> 901 close(control_pipe[1]);
> 902 close(control_pipe[0]);
>
> CID 158927 (#1 of 1): Resource leak (RESOURCE_LEAK)
> 23. leaked_storage: Variable info going out of scope leaks the storage
> it points to.
> 903 return ret;
> 904
> 905umount:
> 21. Condition umount(mi->mountpoint) < 0, taking true branch
> 906 if (umount(mi->mountpoint) < 0)
> 907 pr_perror("Failed to umount %s", mi->mountpoint);
> 22. Jumping to label close_pipe
> 908 goto close_pipe;
> 909}
>
>
>
> 821int autofs_mount(struct mount_info *mi, const char *source, const
> 822 char *filesystemtype, unsigned long mountflags)
> 823{
> 824 AutofsEntry *entry;
> 825 autofs_info_t *info;
> 826 char *opts, *mode;
> 827 int control_pipe[2], ret = -1;
> 828 struct stat buf;
> 829
> 1. Condition autofs_restore_entry(mi, &entry) < 0, taking false
> branch
> 830 if (autofs_restore_entry(mi, &entry) < 0)
> 831 return -1;
> 832
> 2. Condition pipe(control_pipe) < 0, taking false branch
> 833 if (pipe(control_pipe) < 0) {
> 834 pr_perror("Can't create pipe");
> 835 return -1;
> 836 }
> 837
> 838 mode = "direct";
> 3. Condition entry->mode == 1, taking true branch
> 839 if (entry->mode == AUTOFS_MODE_INDIRECT)
> 840 mode = "indirect";
> 4. Condition entry->mode == 2, taking false branch
> 841 if (entry->mode == AUTOFS_MODE_OFFSET)
> 842 mode = "offset";
> 843
> 844 opts = xsprintf("fd=%d,pgrp=%d,minproto=%d,maxproto=%d,%s",
> 845 control_pipe[1], getpgrp(), entry->minproto,
> 846 entry->maxproto, mode);
> 5. Condition opts, taking true branch
> 6. Condition entry->has_uid, taking true branch
> 847 if (opts && entry->has_uid)
> 848 opts = xstrcat(opts, ",uid=%d", entry->uid);
> 7. Condition opts, taking true branch
> 8. Condition entry->has_gid, taking true branch
> 849 if (opts && entry->has_gid)
> 850 opts = xstrcat(opts, ",gid=%d", entry->gid);
> 9. Condition !opts, taking false branch
> 851 if (!opts) {
> 852 pr_err("Failed to create options string\n");
> 853 goto close_pipe;
> 854 }
> 855
> 856 pr_info("autofs: mounting to %s with options: \"%s\"\n",
> 857 mi->mountpoint, opts);
> 858
> 10. Condition mount(source, mi->mountpoint, filesystemtype,
> mountflags, opts) < 0, taking false branch
> 859 if (mount(source, mi->mountpoint, filesystemtype, mountflags,
> opts) < 0) {
> 860 pr_perror("Failed to mount autofs to %s",
> mi->mountpoint);
> 861 goto close_pipe;
> 862 }
> 863
> 11. Condition !___p, taking false branch
> 864 info = xmalloc(sizeof(*info));
> 12. Condition !info, taking false branch
> 865 if (!info)
> 866 goto umount;
> 867 info->entry = entry;
> 868
> 869 /* We need autofs dev_id to be able to open direct mount
> point.
> 870 * But we can't call stat in autofs_add_mount_info(), because
> autofs
> 871 * mount can be overmounted. Thus we have to call it here.
> But shared
> 872 * data is not ready yet. So, let's put in on mi->private and
> copy to
> 873 * shared data in autofs_add_mount_info().
> 874 */
>
> CID 158928 (#1 of 1): Time of check time of use (TOCTOU)
> 13. fs_check_call: Calling function stat to perform check on
> mi->mountpoint.
> 14. Condition stat(mi->mountpoint, &buf) < 0, taking true branch
> 875 if (stat(mi->mountpoint, &buf) < 0) {
> 876 pr_perror("Failed to stat %s", mi->mountpoint);
> 15. Jumping to label umount
> 877 goto umount;
> 878 }
> 879 info->mnt_dev = buf.st_dev;
> 880
> 881 /* We need to create dentries for nested mounts */
> 882 ret = autofs_populate_mount(mi, entry);
> 883 if (ret < 0)
> 884 goto umount;
> 885
> 886 /* In case of catatonic mounts all we need as the function
> call below */
> 887 ret = autofs_post_mount(mi->mountpoint, buf.st_dev,
> entry->timeout);
> 888 if (ret < 0)
> 889 goto umount;
> 890
> 891 /* Otherwise we have to add shared object creation callback
> */
> 892 if (entry->fd != AUTOFS_CATATONIC_FD) {
> 893 ret = add_post_prepare_cb(autofs_add_mount_info, mi);
> 894 if (ret < 0)
> 895 goto umount;
> 896 }
> 897
> 898 mi->private = info;
> 899
> 900close_pipe:
> 901 close(control_pipe[1]);
> 902 close(control_pipe[0]);
> CID 158927: Resource leak (RESOURCE_LEAK) [select issue]
> 903 return ret;
> 904
> 905umount:
> 16. toctou: Calling function umount that uses mi->mountpoint
> after a check function. This can cause a time-of-check, time-of-use race
> condition.
> 906 if (umount(mi->mountpoint) < 0)
> 907 pr_perror("Failed to umount %s", mi->mountpoint);
> 908 goto close_pipe;
> 909}
>
> On Wed, Mar 16, 2016 at 04:16:49PM +0300, Stanislav Kinsburskiy wrote:
>> This patch set adds suport for AutoFS mount migration, including:
>> 1) Direct mounts
>> 2) Indirect mounts
>> 3) Catatonic mounts
>>
>> Note: systemd AutoFS logic breaks with this migration, because device value of
>> new mount differs to the one, which systemd collected, when it was created the
>> mount. Because of it systemd ignors any requests from kernel.
>>
>> v8:
>> 1) find_unused_fd() was optimized: if hint_fd is -1, last used fd + 1 is
>> returned without loop iteration.
>> 2) Patch "mount: copy private field to "bind mount" mount_info structure" was
>> dropped as obsolete.
>> 3) Some cleanup in functions naming.
>> 4) Feature "autofs" was added to test descriptor.
>>
>> v7:
>> 1) Rebased on criu-dev branch
>> 2) test attached
>>
>> v6: indirect mounts dentries are now created within autofs code on autofs
>> mount.
>>
>> v5: this version has significant changes in comparison to previous one:
>> 1) Autofs data is migrated via AutofsEntry structure, which is nested to
>> MntEntry structure.
>> 2) Dump stage is now only converts real pgrp into virtual one. All the collect
>> and control pipe check is done in "parse" stage.
>> 3) All the autofs pipes are collected into a list. This list is checked on
>> packetized pipe dump to distinguish between autofs and other pipes (which are
>> still not supported).
>> 4) Control pipe restore is now done by creating another pipe fd with custom
>> post_open callback instead of artificial files.
>> 5) New list of objects and actors was introduced to create shared autofs data
>> after both mounts and shrared objects are restored.
>> 6) Timeout restore and making mount catatonic is done right after mount pint
>> is created. It allows to avoid of autofs data objects creation and post_open
>> callback calling in case of catatonic mounts.
>>
>> v4: mount info is now transfered in deducated autofs images.
>>
>> v3: fixed write pipe end restore, when it was closed.
>> _______________________________________________
>> CRIU mailing list
>> CRIU at openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
More information about the CRIU
mailing list